mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-17 12:22:34 +03:00
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
This commit is contained in:
diegosouzapw
@@ -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<string, unknown>);
|
||||
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<string, unknown>,
|
||||
responseBody: memoryExtractionResponse as Record<string, unknown> | 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<string, unknown>);
|
||||
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<string, unknown> | 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<string, unknown>,
|
||||
responseBody: (streamResponseBody ?? null) as Record<string, unknown> | null,
|
||||
extractFacts,
|
||||
log,
|
||||
});
|
||||
}
|
||||
|
||||
// Semantic cache: store assembled streaming response for future cache hits
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
const clone: Record<string, unknown> = { ...source };
|
||||
let rootClone: Record<string, unknown> | null = null;
|
||||
let redacted = false;
|
||||
const clonedContainers = new Map<string, unknown[]>();
|
||||
const clonedMessages = new Map<string, Record<string, unknown>>();
|
||||
|
||||
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<string, unknown>).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<string, unknown>;
|
||||
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<string, unknown>),
|
||||
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<string, unknown>).content;
|
||||
if (!Array.isArray(originalContent)) continue;
|
||||
|
||||
const messageKey = `${container}:${messageIndex}`;
|
||||
let messageClone = clonedMessages.get(messageKey);
|
||||
if (!messageClone) {
|
||||
messageClone = {
|
||||
...(originalMessage as Record<string, unknown>),
|
||||
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<string, unknown>), text: redactedText };
|
||||
}
|
||||
|
||||
return clone;
|
||||
return redacted && rootClone ? rootClone : body;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -131,17 +131,18 @@ export function resolveMemoryOwnerId(apiKeyInfo: Record<string, unknown> | 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<string, unknown> | null | undefined;
|
||||
responseBody: Record<string, unknown> | 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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -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<typeof buildBody>).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/);
|
||||
}
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
});
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
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);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user