From a9f7598c606ff9fafab43a166b0ff8e3c9fc33f0 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 5 Sep 2026 03:15:25 -0300 Subject: [PATCH] feat(db): fail-closed previous_response_id continuation for redacted video turns (#12150 P2b) (#12707) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merged, with one column-reconciliation gap closed. The fail-closed reasoning is right and the comments carry it well: a stored snapshot whose cues were replaced by `[redacted-video-transcript]` must not be rehydrated as continuation history, because forwarding placeholder text upstream as if it were the client's real turn is worse than making the client resend. Treating it exactly like `previous_response_not_found` means no new client-visible behaviour to document. Migration 173 does not collide — the tip runs to 172. **What I added:** `video_content_removed` to `ensureCallLogsColumns` in `src/lib/db/schemaColumns.ts`, plus a case in `tests/unit/db-schema-columns-split.test.ts`. `resolvePreviousResponseState` now SELECTs that column on every `previous_response_id` lookup. Migration 173 creates it, but this repo carries a separate reconciliation path for lineages that skipped a migration — and on such a database the SELECT would throw `no such column: video_content_removed` instead of failing closed. That is the same hole #12470 closed for `provider_connections.last_ping_at` earlier today, so the pattern was fresh. Verified red-then-green: stubbing the new reconciliation out drops the suite to 8/9; restored, 9/9. Validated on `release/v3.8.51`: `responses-continuation-store`, `save-call-log-persistence`, `video-bridge-log-redaction` and `db-schema-columns-split` all green (54 focused tests, 0 failures). `typecheck:core` and `lint` clean. The integration run logs `[DB] Added call_logs.video_content_removed column`, which is the reconciliation firing on a fresh test database. --- open-sse/handlers/chatCore.ts | 4 ++ open-sse/handlers/chatCore/attemptLogging.ts | 11 +++ .../173_call_logs_video_content_removed.sql | 16 +++++ src/lib/db/responsesContinuationStore.ts | 14 +++- src/lib/db/schemaColumns.ts | 8 +++ src/lib/usage/callLogs.ts | 11 ++- tests/unit/db-schema-columns-split.test.ts | 25 +++++++ .../unit/responses-continuation-store.test.ts | 68 ++++++++++++++++++- tests/unit/save-call-log-persistence.test.ts | 67 ++++++++++++++++++ tests/unit/video-bridge-log-redaction.test.ts | 35 ++++++++++ 10 files changed, 252 insertions(+), 7 deletions(-) create mode 100644 src/lib/db/migrations/173_call_logs_video_content_removed.sql diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index 622e934084..1b99f46cca 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -1097,6 +1097,10 @@ export async function handleChatCore({ // #12150 P1b surface 1: undefined for every non-video request (byte-identical // to before this param existed) — see applyVideoBridgeLogRedaction. videoBridgeLogRedaction: (videoBridgeLog as VideoBridgeLogParam | undefined)?.redaction, + // #12150 P2 surface 2: mark the persisted call_logs row so + // resolvePreviousResponseState refuses to rehydrate a snapshot whose video + // transcript was redacted. false for every non-video request. + videoContentRemoved: videoBridgeObserved, }); // Primary path: merge client model id + alias target so config on either key applies; resolved diff --git a/open-sse/handlers/chatCore/attemptLogging.ts b/open-sse/handlers/chatCore/attemptLogging.ts index 5ae0876f77..3192ddbd9b 100644 --- a/open-sse/handlers/chatCore/attemptLogging.ts +++ b/open-sse/handlers/chatCore/attemptLogging.ts @@ -250,6 +250,15 @@ export type PersistAttemptLogsContext = { * path) is never touched. Omitted/empty for every non-video request. */ videoBridgeLogRedaction?: VideoBridgeLogRedactionEntry[]; + /** + * #12150 P2 surface 2: true when the video-bridge guardrail observed and + * rewrote video parts on this request, so the persisted client snapshot had + * its transcript cues structurally redacted (videoBridgeObserved in + * chatCore.ts). Written to the `call_logs.video_content_removed` marker so + * `resolvePreviousResponseState` refuses to rehydrate this row as continuation + * history. Omitted/false for every non-video request. + */ + videoContentRemoved?: boolean; }; function toConnectionId(value: unknown): string | null { @@ -368,6 +377,7 @@ export function persistAttemptLogs(args: PersistAttemptLogsArgs, ctx: PersistAtt modelPinned, sessionTag, videoBridgeLogRedaction, + videoContentRemoved, } = ctx; const initialConnectionId = toConnectionId(connectionId); const finalConnectionId = toConnectionId(credentials?.connectionId) || initialConnectionId; @@ -499,6 +509,7 @@ export function persistAttemptLogs(args: PersistAttemptLogsArgs, ctx: PersistAtt modelPinned: modelPinned || false, sessionTag: sessionTag || null, responseId: extractResponsesId(sourceFormat, clientResponse), + videoContentRemoved: videoContentRemoved || false, }).catch(() => {}); // Emit the terminal request-lifecycle event to the live dashboard bus. `request.started` diff --git a/src/lib/db/migrations/173_call_logs_video_content_removed.sql b/src/lib/db/migrations/173_call_logs_video_content_removed.sql new file mode 100644 index 0000000000..f6c244e323 --- /dev/null +++ b/src/lib/db/migrations/173_call_logs_video_content_removed.sql @@ -0,0 +1,16 @@ +-- 173: mark call-log rows whose persisted client-request snapshot had its +-- video transcript content structurally redacted (#12150 P2 surface 2). +-- +-- Set to 1 by the call-log write path when the video-bridge guardrail observed +-- and rewrote video parts on this request (see videoBridgeObserved in +-- open-sse/handlers/chatCore.ts). resolvePreviousResponseState +-- (src/lib/db/responsesContinuationStore.ts) refuses to rehydrate a row so +-- marked: the stored snapshot carries [redacted-video-transcript] placeholders +-- in place of the client's real cues, so reconstructing a continuation off it +-- would forward the placeholder text upstream as if it were real history. +-- Failing closed makes the client resend full history instead, exactly like a +-- real previous_response_not_found. +-- +-- Default 0 (NOT NULL): every existing and non-video row is "nothing removed". + +ALTER TABLE call_logs ADD COLUMN video_content_removed INTEGER NOT NULL DEFAULT 0; diff --git a/src/lib/db/responsesContinuationStore.ts b/src/lib/db/responsesContinuationStore.ts index dce175dc92..90d96e8f6b 100644 --- a/src/lib/db/responsesContinuationStore.ts +++ b/src/lib/db/responsesContinuationStore.ts @@ -66,17 +66,27 @@ export function resolvePreviousResponseState( const db = getDbInstance(); const row = db .prepare( - `SELECT artifact_relpath, api_key_id FROM call_logs + `SELECT artifact_relpath, api_key_id, video_content_removed FROM call_logs WHERE response_id = ? AND detail_state = 'ready' ORDER BY timestamp DESC LIMIT 1` ) - .get(responseId) as { artifact_relpath: string | null; api_key_id: string | null } | undefined; + .get(responseId) as + | { artifact_relpath: string | null; api_key_id: string | null; video_content_removed: number } + | undefined; if (!row || !row.artifact_relpath) return null; // Tenant isolation: a response id is only ever handed back to the API key // that created it. A stored row with no api_key_id at all (no-log/legacy) // can never be resolved by any key -- fail closed rather than guess. if (!apiKeyId || row.api_key_id !== apiKeyId) return null; + // #12150 P2 surface 2: the persisted clientRawRequest snapshot on this row had + // its video transcript cues structurally redacted to [redacted-video-transcript] + // before storage (videoBridgeSnapshotRedaction, marker written by the call-log + // path). The stored input therefore no longer carries the client's real cue + // text -- reconstructing a continuation off it would forward the placeholder + // upstream as if it were genuine history. Fail closed so the client resends + // full history, exactly like a real previous_response_not_found. + if (row.video_content_removed === 1) return null; const { artifact, state } = readCallArtifact(row.artifact_relpath); if (state !== "ready" || !artifact?.pipeline) return null; diff --git a/src/lib/db/schemaColumns.ts b/src/lib/db/schemaColumns.ts index 068c140c69..b288072dac 100644 --- a/src/lib/db/schemaColumns.ts +++ b/src/lib/db/schemaColumns.ts @@ -240,6 +240,14 @@ export function ensureCallLogsColumns(db: SqliteDatabase) { db.exec("ALTER TABLE call_logs ADD COLUMN request_summary TEXT DEFAULT NULL"); console.log("[DB] Added call_logs.request_summary column"); } + // added by 173_call_logs_video_content_removed; back-filled here because + // resolvePreviousResponseState SELECTs it on every continuation lookup — a + // lineage that skipped the migration would throw "no such column" there + // rather than fail closed. Same hole #12470 closed for provider_connections. + if (!columnNames.has("video_content_removed")) { + db.exec("ALTER TABLE call_logs ADD COLUMN video_content_removed INTEGER NOT NULL DEFAULT 0"); + console.log("[DB] Added call_logs.video_content_removed column"); + } if (!columnNames.has("correlation_id")) { db.exec("ALTER TABLE call_logs ADD COLUMN correlation_id TEXT DEFAULT NULL"); console.log("[DB] Added call_logs.correlation_id column"); diff --git a/src/lib/usage/callLogs.ts b/src/lib/usage/callLogs.ts index 5f0e3a03fc..3e1ed314de 100644 --- a/src/lib/usage/callLogs.ts +++ b/src/lib/usage/callLogs.ts @@ -522,6 +522,11 @@ async function saveCallLogOperation(entry: any): Promise { // this row's artifact for OmniRoute-native continuation. See // src/lib/db/responsesContinuationStore.ts. responseId: typeof entry.responseId === "string" ? entry.responseId : null, + // #12150 P2 surface 2: 1 when this request's persisted client snapshot had + // its video transcript cues structurally redacted, so + // resolvePreviousResponseState refuses to rehydrate it as continuation + // history. See src/lib/db/responsesContinuationStore.ts. + videoContentRemoved: entry.videoContentRemoved ? 1 : 0, }; const requestSummary = noLogEnabled @@ -570,7 +575,8 @@ async function saveCallLogOperation(entry: any): Promise { combo_name, combo_step_id, combo_execution_key, error_summary, detail_state, artifact_relpath, artifact_size_bytes, artifact_sha256, has_request_body, has_response_body, has_pipeline_details, request_summary, - correlation_id, model_pinned, session_tag, response_id, error_type + correlation_id, model_pinned, session_tag, response_id, error_type, + video_content_removed ) VALUES ( @id, @timestamp, @method, @path, @status, @model, @requestedModel, @provider, @@ -581,7 +587,8 @@ async function saveCallLogOperation(entry: any): Promise { @comboName, @comboStepId, @comboExecutionKey, @errorSummary, @detailState, @artifactRelPath, @artifactSizeBytes, @artifactSha256, @hasRequestBody, @hasResponseBody, @hasPipelineDetails, @requestSummary, - @correlationId, @modelPinned, @sessionTag, @responseId, @errorType + @correlationId, @modelPinned, @sessionTag, @responseId, @errorType, + @videoContentRemoved ) ` ).run({ diff --git a/tests/unit/db-schema-columns-split.test.ts b/tests/unit/db-schema-columns-split.test.ts index 9e7e249a6a..0587817500 100644 --- a/tests/unit/db-schema-columns-split.test.ts +++ b/tests/unit/db-schema-columns-split.test.ts @@ -11,6 +11,7 @@ import { ensureUsageHistoryColumns, ensureProviderConnectionsColumns, ensureProxyLogsColumns, + ensureCallLogsColumns, hasColumn, hasTable, quoteIdentifier, @@ -182,3 +183,27 @@ test("ensureProviderConnectionsColumns back-fills last_ping columns on a pre-123 db.close?.(); } }); + +// #12150 P2b: `resolvePreviousResponseState` SELECTs `video_content_removed` on +// every previous_response_id lookup. Migration 173 adds it, but a lineage that +// skipped 173 would raise "no such column" there instead of failing closed, so +// the reconciliation has to carry it too — the hole #12470 closed for +// provider_connections. +test("ensureCallLogsColumns back-fills video_content_removed on a pre-173 lineage", () => { + const db = openMemoryDb(); + try { + db.exec("CREATE TABLE call_logs (id TEXT PRIMARY KEY, timestamp TEXT)"); + assert.equal(hasColumn(db, "call_logs", "video_content_removed"), false); + + ensureCallLogsColumns(db); + + assert.equal(hasColumn(db, "call_logs", "video_content_removed"), true); + const row = db + .prepare("SELECT video_content_removed AS v FROM call_logs WHERE id = ?") + .get("missing") as { v: number } | undefined; + assert.equal(row, undefined, "empty table — the column just has to be selectable"); + assert.doesNotThrow(() => ensureCallLogsColumns(db)); + } finally { + db.close?.(); + } +}); diff --git a/tests/unit/responses-continuation-store.test.ts b/tests/unit/responses-continuation-store.test.ts index 0b0bce17c3..6d8cb66c5f 100644 --- a/tests/unit/responses-continuation-store.test.ts +++ b/tests/unit/responses-continuation-store.test.ts @@ -27,13 +27,15 @@ function insertCallLog(row: { apiKeyId: string | null; detailState: string; artifactRelPath: string | null; + videoContentRemoved?: 0 | 1; }) { const db = core.getDbInstance(); db.prepare( `INSERT INTO call_logs (id, timestamp, method, path, status, model, provider, account, duration, - tokens_in, tokens_out, api_key_id, detail_state, artifact_relpath, response_id) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)` + tokens_in, tokens_out, api_key_id, detail_state, artifact_relpath, response_id, + video_content_removed) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)` ).run( row.id, new Date().toISOString(), @@ -49,7 +51,8 @@ function insertCallLog(row: { row.apiKeyId, row.detailState, row.artifactRelPath, - row.responseId + row.responseId, + row.videoContentRemoved ?? 0 ); } @@ -398,6 +401,65 @@ test("resolvePreviousResponseState fails closed on an empty output array even wi assert.equal(store.resolvePreviousResponseState("resp_gen-empty-output", "key-1"), null); }); +test("resolvePreviousResponseState fails closed when the row had video content removed (#12150 P2)", () => { + // #12150 P2 surface 2: the persisted clientRawRequest snapshot had its video + // transcript cues structurally redacted to [redacted-video-transcript] before + // storage (videoBridgeSnapshotRedaction). The stored input therefore no longer + // carries the client's real cue text -- reconstructing a continuation off it + // would forward the placeholder upstream as if it were genuine history. When the + // owning row is marked video_content_removed=1 this must fail closed (return + // null) so the client resends full history, exactly like previous_response_not_found, + // even though the artifact itself is otherwise a perfectly resolvable 'ready' row. + insertCallLog({ + id: "log-video-removed", + responseId: "resp_video_removed", + apiKeyId: "key-1", + detailState: "ready", + artifactRelPath: "2026-01-01/log-video-removed.json", + videoContentRemoved: 1, + }); + writeArtifact("2026-01-01/log-video-removed.json", { + clientRawRequest: { + body: { + input: [{ type: "message", role: "user", content: "[redacted-video-transcript]" }], + }, + }, + providerRequest: { body: { input: [] } }, + clientResponse: { + id: "resp_video_removed", + output: [{ type: "message", role: "assistant", content: "hello" }], + }, + }); + + assert.equal(store.resolvePreviousResponseState("resp_video_removed", "key-1"), null); +}); + +test("resolvePreviousResponseState still resolves a normal row (video_content_removed=0)", () => { + // Guard the fail-closed above does not over-fire: an ordinary row (the default + // 0) resolves exactly as before. + insertCallLog({ + id: "log-video-notremoved", + responseId: "resp_video_notremoved", + apiKeyId: "key-1", + detailState: "ready", + artifactRelPath: "2026-01-01/log-video-notremoved.json", + videoContentRemoved: 0, + }); + writeArtifact("2026-01-01/log-video-notremoved.json", { + clientRawRequest: { body: { input: [{ type: "message", role: "user", content: "hi" }] } }, + providerRequest: { body: { input: [{ type: "message", role: "user", content: "hi" }] } }, + clientResponse: { + id: "resp_video_notremoved", + output: [{ type: "message", role: "assistant", content: "hello" }], + }, + }); + + assert.deepEqual(store.resolvePreviousResponseState("resp_video_notremoved", "key-1"), { + input: [{ type: "message", role: "user", content: "hi" }], + output: [{ type: "message", role: "assistant", content: "hello" }], + }); +}); + test("resolvePreviousResponseState returns null when detail logging was never captured for this row", () => { insertCallLog({ id: "log-5", diff --git a/tests/unit/save-call-log-persistence.test.ts b/tests/unit/save-call-log-persistence.test.ts index 8d59ad18d6..7eb0cef2e5 100644 --- a/tests/unit/save-call-log-persistence.test.ts +++ b/tests/unit/save-call-log-persistence.test.ts @@ -152,6 +152,73 @@ test("saveCallLog persists modelPinned=false as 0", async () => { db.prepare("DELETE FROM call_logs WHERE id = ?").run(testId); }); +test("call_logs table has video_content_removed column", () => { + const db = getDbInstance(); + const columns = db.prepare("PRAGMA table_info(call_logs)").all() as { name: string }[]; + const colNames = columns.map((c) => c.name); + assert.ok( + colNames.includes("video_content_removed"), + "call_logs should have video_content_removed column" + ); +}); + +test("saveCallLog persists videoContentRemoved=true as 1 (#12150 P2)", async () => { + const db = getDbInstance(); + const testId = `test-videoremoved-${Date.now()}`; + + await saveCallLog({ + id: testId, + method: "POST", + path: "/v1/responses", + status: 200, + model: "video-model", + provider: "test-provider", + duration: 500, + tokens: { in: 10, out: 5 }, + videoContentRemoved: true, + }); + + const row = db + .prepare("SELECT id, video_content_removed FROM call_logs WHERE id = ?") + .get(testId) as Record; + assert.ok(row, "row should exist"); + assert.equal( + row.video_content_removed, + 1, + "video_content_removed should be 1 when videoContentRemoved=true" + ); + + db.prepare("DELETE FROM call_logs WHERE id = ?").run(testId); +}); + +test("saveCallLog defaults video_content_removed to 0 when absent (#12150 P2)", async () => { + const db = getDbInstance(); + const testId = `test-novideoremoved-${Date.now()}`; + + await saveCallLog({ + id: testId, + method: "POST", + path: "/v1/chat/completions", + status: 200, + model: "normal-model", + provider: "test-provider", + duration: 500, + tokens: { in: 10, out: 5 }, + }); + + const row = db + .prepare("SELECT id, video_content_removed FROM call_logs WHERE id = ?") + .get(testId) as Record; + assert.ok(row, "row should exist"); + assert.equal( + row.video_content_removed, + 0, + "video_content_removed should default to 0 when not provided" + ); + + db.prepare("DELETE FROM call_logs WHERE id = ?").run(testId); +}); + test("getCallLogs returns modelPinned boolean", async () => { const db = getDbInstance(); const testId = `test-pinned-roundtrip-${Date.now()}`; diff --git a/tests/unit/video-bridge-log-redaction.test.ts b/tests/unit/video-bridge-log-redaction.test.ts index 268102d246..d828cb31a4 100644 --- a/tests/unit/video-bridge-log-redaction.test.ts +++ b/tests/unit/video-bridge-log-redaction.test.ts @@ -149,6 +149,41 @@ test("persisted requestBody carries the placeholder and never the raw transcript ); }); +test("#12150 P2 surface 2: persistAttemptLogs marks the call_logs row video_content_removed=1 when ctx.videoContentRemoved is true", async () => { + // The continuation fail-closed (resolvePreviousResponseState) depends on this + // marker being written for any request whose stored client snapshot had its + // video transcript redacted. This proves the ctx.videoContentRemoved signal + // reaches the persisted row; the row is the exact thing the continuation store + // reads back. + const id = "video-marker-1"; + persistAttemptLogs( + { status: 200, tokens: { input: 1, output: 2 } }, + baseCtx({ pendingRequestId: id, videoContentRemoved: true }) + ); + const row = await pollForCallLog(id); + assert.ok(row, "call log row should be persisted"); + const marker = coreDb + .getDbInstance() + .prepare("SELECT video_content_removed FROM call_logs WHERE id = ?") + .get(id) as { video_content_removed: number }; + assert.equal(marker.video_content_removed, 1); +}); + +test("#12150 P2 surface 2: the marker defaults to 0 for an ordinary (non-video) request", async () => { + const id = "video-marker-control-1"; + persistAttemptLogs( + { status: 200, tokens: { input: 1, output: 2 } }, + baseCtx({ pendingRequestId: id }) + ); + const row = await pollForCallLog(id); + assert.ok(row); + const marker = coreDb + .getDbInstance() + .prepare("SELECT video_content_removed FROM call_logs WHERE id = ?") + .get(id) as { video_content_removed: number }; + assert.equal(marker.video_content_removed, 0); +}); + test("control: without a redaction map the persisted requestBody keeps the original text (model path untouched)", async () => { const id = "video-control-1"; persistAttemptLogs(