feat(db): fail-closed previous_response_id continuation for redacted video turns (#12150 P2b) (#12707)

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.
This commit is contained in:
Diego Rodrigues de Sa e Souza
2026-09-05 03:15:25 -03:00
committed by GitHub
parent d345520d72
commit a9f7598c60
10 changed files with 252 additions and 7 deletions

View File

@@ -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

View File

@@ -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`

View File

@@ -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;

View File

@@ -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;

View File

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

View File

@@ -522,6 +522,11 @@ async function saveCallLogOperation(entry: any): Promise<void> {
// 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<void> {
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<void> {
@comboName, @comboStepId, @comboExecutionKey, @errorSummary, @detailState,
@artifactRelPath, @artifactSizeBytes, @artifactSha256,
@hasRequestBody, @hasResponseBody, @hasPipelineDetails, @requestSummary,
@correlationId, @modelPinned, @sessionTag, @responseId, @errorType
@correlationId, @modelPinned, @sessionTag, @responseId, @errorType,
@videoContentRemoved
)
`
).run({

View File

@@ -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?.();
}
});

View File

@@ -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",

View File

@@ -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<string, unknown>;
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<string, unknown>;
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()}`;

View File

@@ -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(