From 380cb2ff3ab7535dc9c0d7d4252fae8017e60b6d Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Wed, 2 Sep 2026 02:39:05 -0300 Subject: [PATCH] fix(video): match log redaction by content and gate response-derived memory for observed requests Fix round 1 (adversarial review) of #12150 P1b. Two findings: CRITICAL: applyVideoBridgeLogRedaction matched the video-bridge redaction map by positional {messageIndex, partIndex}, but those positions are computed by the guardrail's preCall while injectSystemPrompt (prepends a system message when none exists), context-relay handoff injection, and reasoning-rule body rewrites all run afterward and can prepend/splice the message array before persistAttemptLogs serializes the log -- silently invalidating the map. A stale index either misses the video part (transcript logged unredacted) or, worse, overwrites an unrelated legitimate message while the transcript still leaks. Fixed by switching to content-address matching: videoBridge.ts's VideoBridgeLogRedactionEntry now carries fullText (the exact unredacted text placed into the part), and applyVideoBridgeLogRedaction scans every part in the named container for an exact text+type match instead of trusting position. messageIndex/partIndex are kept as advisory/debugging metadata only. New "Scenario A" regression test in video-bridge-log-redaction.test.ts reproduces the real injectSystemPrompt shape and proves both the leak and the corruption are fixed. Important: an observed request's response-derived text (the model's own reply, which received the full unredacted transcript) could still populate durable Memory -- only the request-derived text was gated. Extracted the shared decision + extraction wiring into runMemoryExtractionGate (memoryExtraction.ts), which gates both request- and response-derived extractFacts calls behind one shouldExtractMemory() check; chatCore.ts's two call sites (non-streaming, streaming) now each collapse to a single call. video-bridge-memory-suppression.test.ts's hand-mirrored stub was replaced with tests against the real runMemoryExtractionGate, including one isolating the response-derived path specifically. Refs #12150 --- open-sse/handlers/chatCore.ts | 106 ++++------- open-sse/handlers/chatCore/attemptLogging.ts | 92 ++++++--- .../handlers/chatCore/memoryExtraction.ts | 87 ++++++++- src/lib/guardrails/videoBridge.ts | 25 +++ tests/unit/guardrails/videoBridge.test.ts | 17 +- tests/unit/video-bridge-log-redaction.test.ts | 104 +++++++++- .../video-bridge-memory-suppression.test.ts | 177 +++++++++++------- 7 files changed, 430 insertions(+), 178 deletions(-) diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index bbe358da63..d2d3b3ad8b 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -123,12 +123,7 @@ export { buildStreamingResponseHeaders, stripStaleForwardingHeaders, }; -import { - extractMemoryTextFromResponse, - extractMemoryTextFromRequestBody, - resolveMemoryOwnerId, - shouldExtractMemory, -} from "./chatCore/memoryExtraction.ts"; +import { resolveMemoryOwnerId, runMemoryExtractionGate } from "./chatCore/memoryExtraction.ts"; import { CORS_HEADERS } from "../utils/cors.ts"; import { checkResourcePressureGuard } from "../utils/resourcePressure.ts"; import { normalizeHeaders } from "../utils/headers.ts"; @@ -555,8 +550,9 @@ export async function handleChatCore({ }) { let { provider, model, extendedContext } = modelInfo; // #12150 P1b: true iff the video-bridge guardrail rendered >=1 transcript - // cue into a replaced part of this request. Gates request-derived Memory - // extraction (chatCore/memoryExtraction.ts::shouldExtractMemory). + // cue into a replaced part of this request. Gates both request- and + // response-derived Memory extraction + // (chatCore/memoryExtraction.ts::runMemoryExtractionGate). const videoBridgeObserved: boolean = (videoBridgeLog as VideoBridgeLogParam | undefined)?.observed === true; const resilienceSettings = resolveResilienceSettings(cachedSettings); @@ -5163,36 +5159,22 @@ export async function handleChatCore({ } ); - if (memoryOwnerId && memorySettings?.enabled && memorySettings.maxTokens > 0) { - // #12150 P1b surface 3: a video-bridge-observed request's request-derived - // text is a flattened transcript description, not user-authored - // conversation — never persist it into durable Memory. The - // response-derived extraction just below (the model's own reply) is - // unaffected — out of scope for this gap-closure task. - if ( - shouldExtractMemory({ - enabled: memorySettings.enabled, - maxTokens: memorySettings.maxTokens, - memoryOwnerId, - videoBridgeObserved, - }) - ) { - const requestMemoryText = extractMemoryTextFromRequestBody(body as Record); - if (requestMemoryText) { - extractFacts(requestMemoryText, memoryOwnerId, pipelineSessionId); - } - } else if (videoBridgeObserved) { - log?.debug?.( - "MEMORY", - "Skipping request-derived memory extraction: video-bridge transcript observed" - ); - } - - const memoryText = extractMemoryTextFromResponse(memoryExtractionResponse); - if (memoryText) { - extractFacts(memoryText, memoryOwnerId, pipelineSessionId); - } - } + // #12150 P1b surface 3 (fix round 1): a video-bridge-observed request's + // request- AND response-derived text both carry the full transcript (the + // flattened description on the request side, the model's own reply on + // the response side) — neither may populate durable Memory. See + // runMemoryExtractionGate for the shared gate + extraction wiring, unit + // tested directly in tests/unit/video-bridge-memory-suppression.test.ts. + runMemoryExtractionGate({ + memoryOwnerId, + memorySettings, + videoBridgeObserved, + pipelineSessionId, + requestBody: body as Record, + responseBody: memoryExtractionResponse as Record | null, + extractFacts, + log, + }); const customSkillExecutionEnabled = Boolean(memoryOwnerId) && memorySettings?.skillsEnabled === true; @@ -5806,40 +5788,20 @@ export async function handleChatCore({ }); // === /Quota Share POST-hook streaming === - if ( - memoryOwnerId && - memorySettings?.enabled && - memorySettings.maxTokens > 0 && - streamStatus === 200 - ) { - // #12150 P1b surface 3: see the matching non-streaming gate above — - // suppresses only the request-derived extraction for an observed - // request; the streamed-response extraction just below is unaffected. - if ( - shouldExtractMemory({ - enabled: memorySettings.enabled, - maxTokens: memorySettings.maxTokens, - memoryOwnerId, - videoBridgeObserved, - }) - ) { - const requestMemoryText = extractMemoryTextFromRequestBody(body as Record); - if (requestMemoryText) { - extractFacts(requestMemoryText, memoryOwnerId, pipelineSessionId); - } - } else if (videoBridgeObserved) { - log?.debug?.( - "MEMORY", - "Skipping request-derived memory extraction: video-bridge transcript observed" - ); - } - - const streamedMemoryText = extractMemoryTextFromResponse( - (streamResponseBody ?? null) as Record | null - ); - if (streamedMemoryText) { - extractFacts(streamedMemoryText, memoryOwnerId, pipelineSessionId); - } + if (streamStatus === 200) { + // #12150 P1b surface 3 (fix round 1): see the matching non-streaming + // gate above — an observed request populates NO durable memory from + // either the request-derived text or this streamed response. + runMemoryExtractionGate({ + memoryOwnerId, + memorySettings, + videoBridgeObserved, + pipelineSessionId, + requestBody: body as Record, + responseBody: (streamResponseBody ?? null) as Record | null, + extractFacts, + log, + }); } // Semantic cache: store assembled streaming response for future cache hits diff --git a/open-sse/handlers/chatCore/attemptLogging.ts b/open-sse/handlers/chatCore/attemptLogging.ts index e2effde2ac..2a583896a7 100644 --- a/open-sse/handlers/chatCore/attemptLogging.ts +++ b/open-sse/handlers/chatCore/attemptLogging.ts @@ -35,6 +35,21 @@ import { attachLogMeta } from "./cacheUsageMeta.ts"; * original objects. Returns `body` unchanged (same reference, no allocation) * when there is nothing to redact, so the common non-video path is * byte-identical to before this function existed. + * + * #12150 fix round 1 (adversarial review, CRITICAL): matches by CONTENT + * (`entry.fullText === part.text`), never by `entry.messageIndex`/ + * `entry.partIndex`. Those positions are computed by the guardrail's preCall, + * but request-mutation stages that run AFTER it and BEFORE this log write — + * `injectSystemPrompt` (prepends a message when no system/developer message + * exists), context-relay handoff injection, reasoning-rule body rewrites — + * can prepend or splice the message array, silently invalidating any + * positional index. A stale index either misses the real part (the + * transcript is logged unredacted) or, worse, lands on and overwrites an + * unrelated legitimate message. Scanning every part in the named container + * for an exact text match finds the video part wherever it ended up and + * never touches a part whose text differs — see + * `tests/unit/video-bridge-log-redaction.test.ts`'s "Scenario A" test for the + * reproduction this fixes. */ export function applyVideoBridgeLogRedaction( body: unknown, @@ -44,45 +59,64 @@ export function applyVideoBridgeLogRedaction( if (!body || typeof body !== "object") return body; const source = body as Record; - const clone: Record = { ...source }; + let rootClone: Record | null = null; + let redacted = false; const clonedContainers = new Map(); const clonedMessages = new Map>(); for (const entry of redaction) { - const { container, messageIndex, partIndex, redactedText } = entry; + const { container, fullText, redactedText } = entry; + if (typeof fullText !== "string" || fullText.length === 0) continue; const originalContainer = source[container]; if (!Array.isArray(originalContainer)) continue; + // Mirrors the exact `type` replaceVideoParts() writes for this container + // (videoBridgeHelpers.ts) — a stronger anchor than a loose "text-like" + // check, at zero extra cost. + const expectedPartType = container === "input" ? "input_text" : "text"; - let containerClone = clonedContainers.get(container); - if (!containerClone) { - containerClone = [...originalContainer]; - clonedContainers.set(container, containerClone); - clone[container] = containerClone; + for (let messageIndex = 0; messageIndex < originalContainer.length; messageIndex++) { + const originalMessage = originalContainer[messageIndex]; + if (!originalMessage || typeof originalMessage !== "object") continue; + const originalContent = (originalMessage as Record).content; + if (!Array.isArray(originalContent)) continue; + + for (let partIndex = 0; partIndex < originalContent.length; partIndex++) { + const originalPart = originalContent[partIndex]; + if (!originalPart || typeof originalPart !== "object") continue; + const partRecord = originalPart as Record; + if (partRecord.type !== expectedPartType) continue; + if (partRecord.text !== fullText) continue; + + // Content-address match — clone the path down to this part lazily + // (root -> container array -> this message -> its content array), + // leaving every other sibling on the original references. + if (!rootClone) rootClone = { ...source }; + let containerClone = clonedContainers.get(container); + if (!containerClone) { + containerClone = [...originalContainer]; + clonedContainers.set(container, containerClone); + rootClone[container] = containerClone; + } + + const messageKey = `${container}:${messageIndex}`; + let messageClone = clonedMessages.get(messageKey); + if (!messageClone) { + messageClone = { + ...(originalMessage as Record), + content: [...originalContent], + }; + clonedMessages.set(messageKey, messageClone); + containerClone[messageIndex] = messageClone; + } + + const contentClone = messageClone.content as unknown[]; + contentClone[partIndex] = { ...partRecord, text: redactedText }; + redacted = true; + } } - - const originalMessage = originalContainer[messageIndex]; - if (!originalMessage || typeof originalMessage !== "object") continue; - const originalContent = (originalMessage as Record).content; - if (!Array.isArray(originalContent)) continue; - - const messageKey = `${container}:${messageIndex}`; - let messageClone = clonedMessages.get(messageKey); - if (!messageClone) { - messageClone = { - ...(originalMessage as Record), - content: [...originalContent], - }; - clonedMessages.set(messageKey, messageClone); - containerClone[messageIndex] = messageClone; - } - - const contentClone = messageClone.content as unknown[]; - const originalPart = originalContent[partIndex]; - if (!originalPart || typeof originalPart !== "object") continue; - contentClone[partIndex] = { ...(originalPart as Record), text: redactedText }; } - return clone; + return redacted && rootClone ? rootClone : body; } /** diff --git a/open-sse/handlers/chatCore/memoryExtraction.ts b/open-sse/handlers/chatCore/memoryExtraction.ts index 281e0b4b00..c92fd09d3e 100644 --- a/open-sse/handlers/chatCore/memoryExtraction.ts +++ b/open-sse/handlers/chatCore/memoryExtraction.ts @@ -131,17 +131,18 @@ export function resolveMemoryOwnerId(apiKeyInfo: Record | null) } /** - * Pure decision for whether the request-derived text should be extracted into - * durable Memory (#12150 P1b, surface 3). Extracted from chatCore.ts's inline + * Pure decision for whether durable Memory should be extracted from this + * request at all (#12150 P1b, surface 3). Wraps chatCore.ts's original inline * `memoryOwnerId && memorySettings?.enabled && memorySettings.maxTokens > 0` - * check (unchanged) plus one new condition: a video-bridge-observed request's - * request-derived text is a flattened transcript description, not - * user-authored conversation, so it must never be persisted as a "memory - * fact". `videoBridgeObserved` is optional and defaults to falsy, so every - * existing non-video caller (which never passes it) keeps today's exact - * behavior. Governs ONLY the request-derived extractFacts call — the - * response-derived one (the model's own reply) is out of scope and - * unaffected by this function. + * check (unchanged) plus one new condition: a video-bridge-observed request + * must never populate durable Memory — not from its request-derived text (a + * flattened transcript description, not user-authored conversation) and, per + * fix round 1 (adversarial review), not from its response-derived text + * either, since the model's own reply also received the full transcript and + * can echo it back. `videoBridgeObserved` is optional and defaults to falsy, + * so every existing non-video caller (which never passes it) keeps today's + * exact behavior. See `runMemoryExtractionGate` below for the call-site + * wiring that applies this decision to both extraction sources at once. */ export function shouldExtractMemory(input: { enabled: boolean | null | undefined; @@ -156,3 +157,69 @@ export function shouldExtractMemory(input: { if (videoBridgeObserved) return false; return true; } + +/** + * Runs the full request+response Memory-extraction gate shared by + * chatCore.ts's non-streaming and streaming completion paths (#12150 P1b fix + * round 1). Extracted so this wiring — not just the pure `shouldExtractMemory` + * decision — is unit-testable against the REAL + * `extractMemoryTextFromRequestBody`/`extractMemoryTextFromResponse`, rather + * than a test file hand-mirroring the call sites' shape. + * + * `extractFacts` is injected (not imported directly) purely for testability — + * production callers pass the real `@/lib/memory/extraction` one. When + * `shouldExtractMemory` says no (memory disabled/unconfigured, OR a + * video-bridge-observed request), this is a complete no-op: neither the + * request- nor the response-derived text is extracted, so an observed + * request populates NO durable memory from either source. + */ +export function runMemoryExtractionGate(input: { + memoryOwnerId: string | null | undefined; + memorySettings: { enabled?: boolean | null; maxTokens?: number | null } | null | undefined; + videoBridgeObserved: boolean; + pipelineSessionId: string; + requestBody: Record | null | undefined; + responseBody: Record | null | undefined; + extractFacts: (text: string, memoryOwnerId: string, sessionId: string) => void; + log?: { debug?: (tag: string, message: string) => void } | null; +}): void { + const { + memoryOwnerId, + memorySettings, + videoBridgeObserved, + pipelineSessionId, + requestBody, + responseBody, + extractFacts, + log, + } = input; + if (!memoryOwnerId) return; + + const allowed = shouldExtractMemory({ + enabled: memorySettings?.enabled, + maxTokens: memorySettings?.maxTokens, + memoryOwnerId, + videoBridgeObserved, + }); + if (!allowed) { + // Only worth a log line for the video-bridge case — memory being + // disabled/unconfigured entirely is the normal, silent, non-video path. + if (videoBridgeObserved && memorySettings?.enabled) { + log?.debug?.( + "MEMORY", + "Skipping request+response memory extraction: video-bridge transcript observed" + ); + } + return; + } + + const requestMemoryText = extractMemoryTextFromRequestBody(requestBody ?? null); + if (requestMemoryText) { + extractFacts(requestMemoryText, memoryOwnerId, pipelineSessionId); + } + + const responseMemoryText = extractMemoryTextFromResponse(responseBody ?? null); + if (responseMemoryText) { + extractFacts(responseMemoryText, memoryOwnerId, pipelineSessionId); + } +} diff --git a/src/lib/guardrails/videoBridge.ts b/src/lib/guardrails/videoBridge.ts index 0e1a847013..f8425a188c 100644 --- a/src/lib/guardrails/videoBridge.ts +++ b/src/lib/guardrails/videoBridge.ts @@ -38,11 +38,31 @@ export type { VideoAnalysisContext } from "./videoBridgePipeline"; * `redactedText` is the structured-redaction shadow (see * `DescribedVideo.descriptionRedacted`) for that same part — never derived * from the model-bound text, so it cannot be bypassed by cue content. + * + * #12150 fix round 1 (adversarial review): the downstream log-redaction + * consumer (`applyVideoBridgeLogRedaction`, chatCore/attemptLogging.ts) + * matches by CONTENT (`fullText`), not by `messageIndex`/`partIndex`. + * Between this guardrail's preCall and the eventual log write, other + * request-mutation stages (system-prompt injection when no existing system + * message is found, context-relay handoff injection, reasoning-rule body + * rewrites) can prepend/splice messages, silently invalidating any + * positional index. `messageIndex`/`partIndex` are kept as advisory/ + * debugging metadata only — never used for matching. */ export interface VideoBridgeLogRedactionEntry { container: "messages" | "input"; + /** Advisory only (see interface doc) — may be stale by the time the log is written. */ messageIndex: number; + /** Advisory only (see interface doc) — may be stale by the time the log is written. */ partIndex: number; + /** + * The exact, unredacted text placed into the replaced part + * (`descriptions[i]`, identical to what `replaceVideoParts` writes to + * `content[partIndex].text`). The downstream consumer matches parts by + * `part.text === fullText`, so it finds the video part wherever a later + * stage moved it, and never touches a part whose text differs. + */ + fullText: string; redactedText: string; } @@ -210,6 +230,11 @@ export class VideoBridgeGuardrail extends BaseGuardrail { container: part.container, messageIndex: part.messageIndex, partIndex: part.partIndex, + // The exact text `replaceVideoParts` is about to write into + // content[partIndex].text (same `result.description` value pushed + // to `descriptions` just above) — the content-address key the + // downstream consumer matches on. See the interface doc. + fullText: result.description, redactedText: result.descriptionRedacted, }); } diff --git a/tests/unit/guardrails/videoBridge.test.ts b/tests/unit/guardrails/videoBridge.test.ts index 0502016864..fa62495eba 100644 --- a/tests/unit/guardrails/videoBridge.test.ts +++ b/tests/unit/guardrails/videoBridge.test.ts @@ -176,11 +176,17 @@ test("reports only validated transcript provenance in guardrail metadata and car // must mark itself observed and hand a redaction map keyed to the exact // replaced part, with the placeholder in and the secret text out. assert.equal(result.meta?.videoBridgeObserved, true); + // #12150 fix round 1: the downstream consumer matches by content + // (`fullText`), not by messageIndex/partIndex (see the interface doc on + // VideoBridgeLogRedactionEntry) — assert fullText is present and is the + // exact unredacted text that was placed into the part, alongside the + // still-positional (now advisory) fields and the placeholder text. assert.deepEqual(result.meta?.videoBridgeLogRedaction, [ { container: "messages", messageIndex: 0, partIndex: 0, + fullText: "[Video description: caption; transcript[source=client] spoken words]", redactedText: "[Video description: caption; transcript[source=client] [redacted-video-transcript]]", }, @@ -528,8 +534,17 @@ test("real Video Bridge cache hit preserves the redacted transcript shadow acros for (const result of [first, second]) { assert.equal(result.meta?.videoBridgeObserved, true); const redaction = result.meta?.videoBridgeLogRedaction as - Array<{ redactedText: string }> | undefined; + Array<{ fullText: string; redactedText: string }> | undefined; assert.equal(redaction?.length, 1); + // #12150 fix round 1: fullText must be the exact unredacted text that + // landed in the replaced part — the content-address key the downstream + // consumer matches on — verified against the guardrail's own + // modifiedPayload rather than a hardcoded string (the rendered text + // here comes from the real describeVideoPart pipeline, not a fixture). + const modifiedPart = (result.modifiedPayload as ReturnType).messages[0] + .content[0] as { text: string }; + assert.equal(redaction?.[0]?.fullText, modifiedPart.text); + assert.doesNotMatch(redaction?.[0]?.fullText ?? "", /\[redacted-video-transcript\]/); assert.match(redaction?.[0]?.redactedText ?? "", /\[redacted-video-transcript\]/); assert.doesNotMatch(redaction?.[0]?.redactedText ?? "", /cached secret cue/); } diff --git a/tests/unit/video-bridge-log-redaction.test.ts b/tests/unit/video-bridge-log-redaction.test.ts index 2c32ead1ba..82a934bdc8 100644 --- a/tests/unit/video-bridge-log-redaction.test.ts +++ b/tests/unit/video-bridge-log-redaction.test.ts @@ -11,6 +11,15 @@ // and the caller's own `body` object is never mutated in the process (the // model already received the untouched original earlier in the request // lifecycle; this call must not reach back and change it). +// +// #12150 fix round 1 (adversarial review, CRITICAL): also proves the +// content-address fix for the positional-drift bug — real request-mutation +// stages (injectSystemPrompt's "no existing system message" branch, +// context-relay handoff injection, reasoning-rule body rewrites) can +// prepend/splice messages between the guardrail's preCall and this log +// write, making a stale (messageIndex, partIndex) point at the wrong message +// or an out-of-bounds slot. applyVideoBridgeLogRedaction must locate the +// video part by matching `fullText` against part text, not by position. import { test, before, after } from "node:test"; import assert from "node:assert/strict"; import fs from "node:fs"; @@ -25,6 +34,7 @@ const { getCallLogById } = await import("../../src/lib/usage/callLogs.ts"); const { persistAttemptLogs } = await import("../../open-sse/handlers/chatCore/attemptLogging.ts"); const SECRET = "secret words"; +const FULL_TEXT = `[Video 1]: A person talks. transcript[00:00-00:02]: ${SECRET}`; const PLACEHOLDER_TEXT = "[Video 1]: A person talks. transcript[00:00-00:02]: [redacted-video-transcript]"; @@ -39,7 +49,7 @@ function videoBody() { { type: "text", text: "look at this video" }, { type: "text", - text: `[Video 1]: A person talks. transcript[00:00-00:02]: ${SECRET}`, + text: FULL_TEXT, }, ], }, @@ -105,7 +115,13 @@ test("persisted requestBody carries the placeholder and never the raw transcript baseCtx({ pendingRequestId: id, videoBridgeLogRedaction: [ - { container: "messages", messageIndex: 1, partIndex: 1, redactedText: PLACEHOLDER_TEXT }, + { + container: "messages", + messageIndex: 1, + partIndex: 1, + fullText: FULL_TEXT, + redactedText: PLACEHOLDER_TEXT, + }, ], }) ); @@ -146,7 +162,13 @@ test("the caller's body object is never mutated by the redaction", async () => { pendingRequestId: id, body, videoBridgeLogRedaction: [ - { container: "messages", messageIndex: 1, partIndex: 1, redactedText: PLACEHOLDER_TEXT }, + { + container: "messages", + messageIndex: 1, + partIndex: 1, + fullText: FULL_TEXT, + redactedText: PLACEHOLDER_TEXT, + }, ], }) ); @@ -157,3 +179,79 @@ test("the caller's body object is never mutated by the redaction", async () => { "ctx.body must be byte-identical after persistAttemptLogs runs" ); }); + +test("Scenario A (adversarial review): a message prepended AFTER the guardrail built the redaction map does not leak the transcript, and the prepended message is untouched", async () => { + const id = "video-scenario-a-1"; + + // The body exactly as the video-bridge guardrail saw it when it computed + // the redaction map: a single user message, no system message yet — this + // is precisely the shape that makes injectSystemPrompt's "no existing + // system message" branch (open-sse/services/systemPrompt.ts) fire. + const userMessageWithVideo = { + role: "user", + content: [ + { type: "text", text: "look at this video" }, + { type: "text", text: FULL_TEXT }, + ], + }; + // The map the guardrail built, correct AT THAT MOMENT: the video part was + // messages[0].content[1]. + const redactionMap = [ + { + container: "messages" as const, + messageIndex: 0, + partIndex: 1, + fullText: FULL_TEXT, + redactedText: PLACEHOLDER_TEXT, + }, + ]; + + // Real production shape: AFTER the guardrail ran, injectSystemPrompt found + // no existing system/developer message and unshifted a brand-new one — + // `result.messages = [{ role: "system", content: combined }, ...result.messages]` + // — shifting the video message from index 0 to index 1. The map above is + // now stale by the time persistAttemptLogs serializes the log: a purely + // positional lookup at (messageIndex: 0, partIndex: 1) would land on this + // new system message instead. + const bodyAfterSystemPromptInjection = { + messages: [{ role: "system", content: "You are a helpful assistant." }, userMessageWithVideo], + }; + + persistAttemptLogs( + { status: 200 }, + baseCtx({ + pendingRequestId: id, + body: bodyAfterSystemPromptInjection, + videoBridgeLogRedaction: redactionMap, + }) + ); + + const row = await pollForCallLog(id); + assert.ok(row, "call log row should be persisted"); + const persisted = row.requestBody as { + messages: Array<{ role: string; content: unknown }>; + }; + + // (A) the leak: the video part, now shifted to index 1, must still be + // found and redacted by content, not silently skipped. + const shiftedContent = persisted.messages[1].content as Array<{ text: string }>; + assert.equal( + shiftedContent[1].text, + PLACEHOLDER_TEXT, + "the shifted video part must still be redacted despite the stale positional map" + ); + assert.ok( + !shiftedContent[1].text.includes(SECRET), + "the shifted video part must not leak the raw transcript" + ); + assert.equal(JSON.stringify(persisted).includes(SECRET), false); + + // (B) the corruption: the newly prepended system message — which a + // positional lookup at the stale index would have landed on — must be + // completely untouched. + assert.equal( + persisted.messages[0].content, + "You are a helpful assistant.", + "the prepended system message must be untouched" + ); +}); diff --git a/tests/unit/video-bridge-memory-suppression.test.ts b/tests/unit/video-bridge-memory-suppression.test.ts index 317f315ff3..1b2a4ac102 100644 --- a/tests/unit/video-bridge-memory-suppression.test.ts +++ b/tests/unit/video-bridge-memory-suppression.test.ts @@ -1,25 +1,25 @@ // tests/unit/video-bridge-memory-suppression.test.ts // P1b of #12150 (Video Bridge transcript retention) — surface 3 (Memory sink). // -// chatCore.ts gates its two extractFacts(requestMemoryText, ...) call sites -// (non-streaming ~L5134, streaming ~L5758) on an inline -// `memoryOwnerId && memorySettings?.enabled && memorySettings.maxTokens > 0` -// check. This adds a fourth condition — the request must not be a -// video-bridge-observed one — extracted to a pure, exported decision function -// so it is unit-testable without invoking the handleChatCore monolith (same -// god-file-decomposition convention as chatCore/attemptLogging.ts, -// chatCore/nonStreamingUsageStats.ts, etc.). +// chatCore.ts's two Memory-extraction call sites (non-streaming + streaming) +// now each delegate to a single `runMemoryExtractionGate` (extracted so this +// wiring — not just the underlying `shouldExtractMemory` decision — is +// unit-testable against the REAL extractMemoryTextFromRequestBody/ +// extractMemoryTextFromResponse, same god-file-decomposition convention as +// chatCore/attemptLogging.ts, chatCore/nonStreamingUsageStats.ts, etc.). // -// Only the REQUEST-derived extractFacts call is gated (per the design doc, -// "surface 3: gate extractFacts on !videoBridgeObserved for request-derived -// text") — the response-derived extractFacts call (the model's own reply) is -// out of scope and untouched. +// #12150 fix round 1 (adversarial review, Important): a video-bridge-observed +// request must populate NO durable memory from EITHER source — the +// request-derived text (a flattened transcript description) AND the +// response-derived text (the model's own reply, which also received the full +// transcript and can echo it back). Both are gated by the same +// shouldExtractMemory() decision inside runMemoryExtractionGate. import test from "node:test"; import assert from "node:assert/strict"; import { shouldExtractMemory, - extractMemoryTextFromRequestBody, + runMemoryExtractionGate, } from "../../open-sse/handlers/chatCore/memoryExtraction.ts"; // ─── shouldExtractMemory: pure decision table ────────────────────────────── @@ -95,45 +95,14 @@ test("shouldExtractMemory: still false when memoryOwnerId is null, regardless of ); }); -// ─── Integration stub: wire the real decision + the real request-text ───── -// extractor together against a stubbed extractFacts, mirroring the exact -// shape of the two chatCore.ts call sites (only the DB-writing extractFacts -// is stubbed — everything else is the real exported implementation). +// ─── runMemoryExtractionGate: the REAL chatCore.ts call-site wiring ──────── +// No hand-mirrored stub — this imports and calls the exact function both +// chatCore.ts completion paths call. Only `extractFacts` (the DB-writing, +// fire-and-forget side effect) is injected as a spy; extraction of the +// request/response text runs through the real +// extractMemoryTextFromRequestBody/extractMemoryTextFromResponse. -function runRequestMemoryExtractionStub(params: { - memoryOwnerId: string | null; - memorySettings: { enabled: boolean; maxTokens: number }; - videoBridgeObserved: boolean; - body: Record; - pipelineSessionId: string; - extractFactsSpy: (text: string, ownerId: string, sessionId: string) => void; -}): void { - const { - memoryOwnerId, - memorySettings, - videoBridgeObserved, - body, - pipelineSessionId, - extractFactsSpy, - } = params; - if (memoryOwnerId && memorySettings?.enabled && memorySettings.maxTokens > 0) { - if ( - shouldExtractMemory({ - enabled: memorySettings.enabled, - maxTokens: memorySettings.maxTokens, - memoryOwnerId, - videoBridgeObserved, - }) - ) { - const requestMemoryText = extractMemoryTextFromRequestBody(body); - if (requestMemoryText) { - extractFactsSpy(requestMemoryText, memoryOwnerId, pipelineSessionId); - } - } - } -} - -const flattenedVideoBody = { +const flattenedVideoRequestBody = { messages: [ { role: "user", @@ -142,29 +111,111 @@ const flattenedVideoBody = { ], }; -test("integration stub: zero extractFacts calls for a video-bridge-observed request", () => { +const modelReplyEchoingTranscript = { + choices: [ + { + message: { + content: "Sure — the video shows: secret words", + }, + }, + ], +}; + +function spy() { const calls: Array<[string, string, string]> = []; - runRequestMemoryExtractionStub({ + return { + calls, + fn: (text: string, ownerId: string, sessionId: string) => { + calls.push([text, ownerId, sessionId]); + }, + }; +} + +test("runMemoryExtractionGate: zero extractFacts calls (request AND response) for a video-bridge-observed request", () => { + const extractFacts = spy(); + runMemoryExtractionGate({ memoryOwnerId: "key-1", memorySettings: { enabled: true, maxTokens: 2000 }, videoBridgeObserved: true, - body: flattenedVideoBody, pipelineSessionId: "session-1", - extractFactsSpy: (text, ownerId, sessionId) => calls.push([text, ownerId, sessionId]), + requestBody: flattenedVideoRequestBody, + responseBody: modelReplyEchoingTranscript, + extractFacts: extractFacts.fn, }); - assert.equal(calls.length, 0, "extractFacts must not be called when videoBridgeObserved=true"); + assert.equal( + extractFacts.calls.length, + 0, + "extractFacts must not be called for either source when videoBridgeObserved=true" + ); }); -test("integration stub: extractFacts IS called for the same body when video-bridge was not observed", () => { - const calls: Array<[string, string, string]> = []; - runRequestMemoryExtractionStub({ +test("runMemoryExtractionGate: response-derived extraction specifically is skipped when observed (fix round 1 regression)", () => { + const extractFacts = spy(); + runMemoryExtractionGate({ + memoryOwnerId: "key-1", + memorySettings: { enabled: true, maxTokens: 2000 }, + videoBridgeObserved: true, + pipelineSessionId: "session-1", + // No request-derived text at all (e.g. request body already consumed/ + // reshaped) — isolates the assertion to the response-derived source, + // which is the one fix round 1 found still leaking into Memory. + requestBody: { messages: [] }, + responseBody: modelReplyEchoingTranscript, + extractFacts: extractFacts.fn, + }); + assert.equal( + extractFacts.calls.length, + 0, + "the model's reply (which also received the full transcript) must not be extracted when observed" + ); +}); + +test("runMemoryExtractionGate: extracts BOTH request and response text when video-bridge was not observed", () => { + const extractFacts = spy(); + runMemoryExtractionGate({ memoryOwnerId: "key-1", memorySettings: { enabled: true, maxTokens: 2000 }, videoBridgeObserved: false, - body: flattenedVideoBody, pipelineSessionId: "session-1", - extractFactsSpy: (text, ownerId, sessionId) => calls.push([text, ownerId, sessionId]), + requestBody: flattenedVideoRequestBody, + responseBody: { choices: [{ message: { content: "a normal reply" } }] }, + extractFacts: extractFacts.fn, }); - assert.equal(calls.length, 1, "extractFacts must run on the ordinary (non-video) path"); - assert.match(calls[0][0], /secret words/); + assert.equal( + extractFacts.calls.length, + 2, + "both request- and response-derived extraction run on the ordinary (non-video) path" + ); + assert.match(extractFacts.calls[0][0], /secret words/); + assert.match(extractFacts.calls[1][0], /a normal reply/); + assert.equal(extractFacts.calls[0][1], "key-1"); + assert.equal(extractFacts.calls[0][2], "session-1"); +}); + +test("runMemoryExtractionGate: no-ops when memory is disabled, regardless of videoBridgeObserved", () => { + const extractFacts = spy(); + runMemoryExtractionGate({ + memoryOwnerId: "key-1", + memorySettings: { enabled: false, maxTokens: 2000 }, + videoBridgeObserved: false, + pipelineSessionId: "session-1", + requestBody: flattenedVideoRequestBody, + responseBody: { choices: [{ message: { content: "a normal reply" } }] }, + extractFacts: extractFacts.fn, + }); + assert.equal(extractFacts.calls.length, 0); +}); + +test("runMemoryExtractionGate: no-ops when memoryOwnerId is missing, regardless of videoBridgeObserved", () => { + const extractFacts = spy(); + runMemoryExtractionGate({ + memoryOwnerId: null, + memorySettings: { enabled: true, maxTokens: 2000 }, + videoBridgeObserved: false, + pipelineSessionId: "session-1", + requestBody: flattenedVideoRequestBody, + responseBody: { choices: [{ message: { content: "a normal reply" } }] }, + extractFacts: extractFacts.fn, + }); + assert.equal(extractFacts.calls.length, 0); });