From 8f6d5a278f0bfa455e0bc9d381b82a72d3fae73e Mon Sep 17 00:00:00 2001 From: Joey Krug Date: Sun, 15 Mar 2026 15:51:14 -0400 Subject: [PATCH] fix: use file-block-safe replacement in normalizeUpdatedBody and trailing fallback (#46454) --- src/auto-reply/reply/followup-media.test.ts | 107 ++++++++++++++++++++ src/auto-reply/reply/followup-media.ts | 44 +++++++- 2 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 src/auto-reply/reply/followup-media.test.ts diff --git a/src/auto-reply/reply/followup-media.test.ts b/src/auto-reply/reply/followup-media.test.ts new file mode 100644 index 00000000000..d8ba453f801 --- /dev/null +++ b/src/auto-reply/reply/followup-media.test.ts @@ -0,0 +1,107 @@ +import { describe, expect, it } from "vitest"; +import { + _findLastOccurrenceBeforeFileBlocks as findLastOccurrenceBeforeFileBlocks, + _normalizeUpdatedBody as normalizeUpdatedBody, + _rebuildQueuedPromptWithMediaUnderstanding as rebuildQueuedPromptWithMediaUnderstanding, +} from "./followup-media.js"; + +const FILE_BLOCK = '\nPDF content\n'; + +describe("findLastOccurrenceBeforeFileBlocks", () => { + it("returns -1 for empty search", () => { + expect(findLastOccurrenceBeforeFileBlocks("hello", "")).toBe(-1); + }); + + it("finds last occurrence in body region before file blocks", () => { + const value = `hello world hello\n${FILE_BLOCK}`; + // "hello" appears at 0 and 12 — both before the file block + expect(findLastOccurrenceBeforeFileBlocks(value, "hello")).toBe(12); + }); + + it("does not match inside file block content", () => { + const value = `some text\n${FILE_BLOCK}\nPDF content`; + // "PDF content" appears in the file block and after it, but the body region + // (before { + // When the search string contains a block, it can't appear in the + // body-only region, so the fallback searches the full value. + const bodyWithFile = `caption\n${FILE_BLOCK}`; + const value = `previous\n${bodyWithFile}\nlater\n${bodyWithFile}`; + // Should find the *last* (trailing) occurrence + const expected = value.lastIndexOf(bodyWithFile); + expect(findLastOccurrenceBeforeFileBlocks(value, bodyWithFile)).toBe(expected); + expect(expected).toBeGreaterThan(value.indexOf(bodyWithFile)); + }); + + it("returns index when no file blocks exist in value", () => { + expect(findLastOccurrenceBeforeFileBlocks("abc abc", "abc")).toBe(4); + }); +}); + +describe("normalizeUpdatedBody", () => { + it("returns empty string when updatedBody is empty", () => { + expect(normalizeUpdatedBody({ originalBody: "foo", updatedBody: "" })).toBe(""); + }); + + it("returns updatedBody when originalBody is empty", () => { + expect(normalizeUpdatedBody({ updatedBody: "hello" })).toBe("hello"); + }); + + it("strips directives when updatedBody equals originalBody", () => { + const body = "/think high tell me a joke"; + const result = normalizeUpdatedBody({ originalBody: body, updatedBody: body }); + expect(result).toBe("tell me a joke"); + }); + + it("does not corrupt file block content during directive cleanup", () => { + const originalBody = "/think high tell me about this file"; + // updatedBody has the original body plus a file block appended by media processing + const updatedBody = `${originalBody}\n${FILE_BLOCK}`; + const result = normalizeUpdatedBody({ originalBody, updatedBody }); + // The directive should be stripped from the body portion, file block preserved + expect(result).toContain("tell me about this file"); + expect(result).toContain(FILE_BLOCK); + expect(result).not.toContain("/think"); + }); + + it("replaces in body region, not inside file blocks", () => { + const originalBody = "PDF content"; + const updatedBody = `PDF content\n\nPDF content\n`; + // The replacement should target the body region "PDF content" before the + // file block, not the "PDF content" inside the block. + const result = normalizeUpdatedBody({ originalBody, updatedBody }); + // With no directives to strip, original === cleaned, updatedBody !== originalBody + // because updatedBody has the file block appended. The replacement targets the + // body-region occurrence. + expect(result).toContain('"); + }); +}); + +describe("rebuildQueuedPromptWithMediaUnderstanding", () => { + it("replaces original body with updated body in prompt", () => { + const result = rebuildQueuedPromptWithMediaUnderstanding({ + prompt: "thread context\nhello world", + originalBody: "hello world", + updatedBody: 'hello world\ndata', + }); + expect(result).toContain('data'); + expect(result).toContain("thread context"); + }); + + it("preserves file blocks in thread history when body is replaced", () => { + const prompt = `history\nold\nhello world`; + const result = rebuildQueuedPromptWithMediaUnderstanding({ + prompt, + originalBody: "hello world", + updatedBody: "hello world transcribed", + }); + // The old file block from history should be preserved since updatedBody + // has no file blocks of its own. + expect(result).toContain('old'); + expect(result).toContain("hello world transcribed"); + }); +}); diff --git a/src/auto-reply/reply/followup-media.ts b/src/auto-reply/reply/followup-media.ts index d9202554526..f40ec91f109 100644 --- a/src/auto-reply/reply/followup-media.ts +++ b/src/auto-reply/reply/followup-media.ts @@ -64,6 +64,40 @@ function findFirstOccurrenceBeforeFileBlocks(value: string, search: string): num return bodyRegion.indexOf(search); } +function findLastOccurrenceBeforeFileBlocks(value: string, search: string): number { + if (!search) { + return -1; + } + const fileBlockIndex = value.search(FILE_BLOCK_RE); + const bodyRegion = fileBlockIndex >= 0 ? value.slice(0, fileBlockIndex) : value; + const index = bodyRegion.lastIndexOf(search); + if (index >= 0) { + return index; + } + // Fallback: search string itself contains file blocks — it can't appear in the + // body-only region. Search the full value with lastIndexOf to pick the trailing + // (most recent) occurrence when thread/history has identical bodies. + if (FILE_BLOCK_RE.test(search)) { + return value.lastIndexOf(search); + } + return -1; +} + +function replaceLastOccurrenceBeforeFileBlocks( + value: string, + search: string, + replacement: string, +): string | undefined { + if (!search) { + return undefined; + } + const index = findLastOccurrenceBeforeFileBlocks(value, search); + if (index < 0) { + return undefined; + } + return `${value.slice(0, index)}${replacement}${value.slice(index + search.length)}`; +} + function replaceFirstOccurrenceBeforeFileBlocks( value: string, search: string, @@ -101,7 +135,8 @@ function normalizeUpdatedBody(params: { originalBody?: string; updatedBody?: str return cleanedOriginalBody; } return ( - replaceLastOccurrence(updatedBody, originalBody, cleanedOriginalBody) ?? updatedBody + replaceLastOccurrenceBeforeFileBlocks(updatedBody, originalBody, cleanedOriginalBody) ?? + updatedBody ).trim(); } @@ -215,6 +250,13 @@ function snapshotUpdatedMediaContext(params: { }; } +// Exported for unit testing — these are pure string helpers with no side effects. +export { + findLastOccurrenceBeforeFileBlocks as _findLastOccurrenceBeforeFileBlocks, + normalizeUpdatedBody as _normalizeUpdatedBody, + rebuildQueuedPromptWithMediaUnderstanding as _rebuildQueuedPromptWithMediaUnderstanding, +}; + export async function applyDeferredMediaUnderstandingToQueuedRun( queued: FollowupRun, params: { logLabel?: string } = {},