From c39fab70c16c542486657049ddf4dfad82ff2f1c Mon Sep 17 00:00:00 2001 From: claude_code Date: Thu, 2 Jul 2026 14:31:41 +0300 Subject: [PATCH 1/2] feat(ai-chat): persist page-change diff to history and harden stale-page note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #274 page_changed marker lived only in the ephemeral system prompt, so the diff the agent saw was invisible in the chat export/history, and the note was too weak — the agent still overwrote the user's manual edits with a full-page replace. - Persist the diff the agent saw as metadata.pageChanged on the assistant row (flushAssistant), threaded into all five flush call sites in stream(). Model replay (rowToUiMessage/rowParts) reads only metadata.parts, so the sibling never re-injects the note into the model context on later turns. - Render the persisted diff as a labelled block (en/ru) before the message body in the server-side Markdown export (chat-markdown.util.ts). - Strengthen PAGE_CHANGED_NOTE: mandate a fresh getPage re-read and targeted edits (editPageText/patchNode/insertNode/deleteNode) instead of a whole-page replace, and never revert or overwrite the user's edits. Tests: prompt, export and service specs updated; 114 pass, tsc clean. Co-Authored-By: Claude Opus 4.8 --- .../src/core/ai-chat/ai-chat.prompt.spec.ts | 5 +++ .../server/src/core/ai-chat/ai-chat.prompt.ts | 16 +++++--- .../src/core/ai-chat/ai-chat.service.spec.ts | 26 +++++++++++++ .../src/core/ai-chat/ai-chat.service.ts | 20 ++++++++-- .../core/ai-chat/chat-markdown.util.spec.ts | 38 +++++++++++++++++++ .../src/core/ai-chat/chat-markdown.util.ts | 33 ++++++++++++++++ 6 files changed, 130 insertions(+), 8 deletions(-) diff --git a/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts b/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts index a1e62048..44d976b4 100644 --- a/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts +++ b/apps/server/src/core/ai-chat/ai-chat.prompt.spec.ts @@ -303,6 +303,11 @@ describe('buildSystemPrompt page-changed note (#274)', () => { expect(prompt).toContain(NOTE_MARKER); expect(prompt).toContain('-old line'); expect(prompt).toContain('+new line'); + // Strengthened note (#274): instructs a fresh re-read via getPage and steers + // the agent toward small, targeted edits instead of a full-page overwrite. + expect(prompt).toContain('getPage'); + expect(prompt.toLowerCase()).toContain('targeted'); + expect(prompt).toContain('editPageText'); // Inside the safety sandwich: the trailing SAFETY block follows the note. expect(prompt.lastIndexOf(SAFETY_MARKER)).toBeGreaterThan( prompt.indexOf(NOTE_MARKER), diff --git a/apps/server/src/core/ai-chat/ai-chat.prompt.ts b/apps/server/src/core/ai-chat/ai-chat.prompt.ts index d2b44e15..6fc39473 100644 --- a/apps/server/src/core/ai-chat/ai-chat.prompt.ts +++ b/apps/server/src/core/ai-chat/ai-chat.prompt.ts @@ -85,11 +85,17 @@ const INTERRUPT_NOTE = const PAGE_CHANGED_NOTE = 'NOTE: The user edited the open page AFTER your last response in this ' + 'conversation, so any copy of that page you produced or remember from earlier ' + - 'is now STALE. The unified diff below shows exactly what changed since you last ' + - 'spoke (lines starting with "-" were removed, "+" were added) and is the source ' + - 'of truth. Preserve the user\'s edits: build on the current page, do not revert ' + - 'or overwrite their changes. If you need the full up-to-date page, re-read it ' + - 'with the getPage tool before editing.'; + 'is now STALE and must not be reused. Before you edit the page, you MUST first ' + + 're-read its current content with the getPage tool and base your work on that ' + + 'live version — never on your earlier copy or on the transcript. The unified ' + + 'diff below shows exactly what the user changed since you last spoke (lines ' + + 'starting with "-" were removed, "+" were added) and is the source of truth. ' + + 'Preserve every one of the user\'s edits: make the smallest change that ' + + 'satisfies the request using the targeted edit tools (editPageText, patchNode, ' + + 'insertNode, deleteNode) rather than replacing the whole page, and do not ' + + 'revert, drop, or overwrite anything the user changed. If a full rewrite is ' + + 'truly unavoidable, start from the current getPage content and carry over all ' + + 'of the user\'s edits.'; /** * Sanitize a value interpolated into a prompt XML-ish attribute (e.g. diff --git a/apps/server/src/core/ai-chat/ai-chat.service.spec.ts b/apps/server/src/core/ai-chat/ai-chat.service.spec.ts index a367ec6a..0df4c5dc 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.spec.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.spec.ts @@ -356,6 +356,32 @@ describe('flushAssistant', () => { expect(flushed.toolCalls).not.toBeNull(); expect(flushed.metadata.error).toBe('boom'); }); + + // #274 observability: the page-change diff the agent saw this turn is persisted + // to metadata.pageChanged when a non-empty diff was injected, and omitted when + // the diff is empty/whitespace or the arg is not supplied. + it('persists metadata.pageChanged when a non-empty diff was injected', () => { + const f = flushAssistant([], '', 'completed', { + pageChanged: { title: 'Doc', diff: '@@ -1 +1 @@\n-old\n+new' }, + }); + expect(f.metadata.pageChanged).toEqual({ + title: 'Doc', + diff: '@@ -1 +1 @@\n-old\n+new', + }); + }); + + it('omits metadata.pageChanged for an empty/whitespace diff or a missing arg', () => { + const whitespace = flushAssistant([], '', 'completed', { + pageChanged: { title: 'Doc', diff: ' \n ' }, + }); + expect('pageChanged' in whitespace.metadata).toBe(false); + + const nullArg = flushAssistant([], '', 'completed', { pageChanged: null }); + expect('pageChanged' in nullArg.metadata).toBe(false); + + const omitted = flushAssistant([], '', 'streaming'); + expect('pageChanged' in omitted.metadata).toBe(false); + }); }); /** diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index b2dcbdce..4586185a 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -685,7 +685,7 @@ export class AiChatService implements OnModuleInit { // no-op (guarded below) so the turn still streams to the user. let assistantId: string | undefined; try { - const seed = flushAssistant([], '', 'streaming'); + const seed = flushAssistant([], '', 'streaming', { pageChanged }); const seeded = await this.aiChatMessageRepo.insert({ chatId, workspaceId: workspace.id, @@ -720,7 +720,7 @@ export class AiChatService implements OnModuleInit { await this.aiChatMessageRepo.update( assistantId, workspace.id, - flushAssistant(capturedSteps, '', 'streaming'), + flushAssistant(capturedSteps, '', 'streaming', { pageChanged }), { onlyIfStreaming: true }, ); } catch (err) { @@ -860,6 +860,7 @@ export class AiChatService implements OnModuleInit { // resolved from the admin-configured provider settings (in // closure scope here). Omitted/0 = no limit. maxContextTokens: resolved?.chatContextWindow, + pageChanged, }), ); // Lifecycle: release the external MCP clients leased for this turn. @@ -911,6 +912,7 @@ export class AiChatService implements OnModuleInit { await finalizeAssistant( flushAssistant(capturedSteps, inProgressText, 'error', { error: errorText, + pageChanged, }), ); await closeExternalClients(); @@ -940,7 +942,9 @@ export class AiChatService implements OnModuleInit { `steps=${steps.length}`, ); await finalizeAssistant( - flushAssistant(capturedSteps, inProgressText, 'aborted'), + flushAssistant(capturedSteps, inProgressText, 'aborted', { + pageChanged, + }), ); await closeExternalClients(); // Advance the page snapshot even on abort (#274): an agent edit that @@ -1506,6 +1510,7 @@ export function flushAssistant( contextTokens?: number; maxContextTokens?: number; error?: string; + pageChanged?: { title: string; diff: string } | null; }, ): AssistantFlush { const finished = capturedSteps ?? []; @@ -1538,6 +1543,15 @@ export function flushAssistant( if (extra?.maxContextTokens) metadata.maxContextTokens = extra.maxContextTokens; if (extra?.error) metadata.error = extra.error; + // Persist the page-change diff the agent saw this turn (#274 observability), + // so history / the Markdown export can show what the user changed. Only when + // a non-empty diff was actually injected into the prompt this turn. + if (extra?.pageChanged && extra.pageChanged.diff?.trim().length) { + metadata.pageChanged = { + title: extra.pageChanged.title, + diff: extra.pageChanged.diff, + }; + } return { content: stepsText + trailing, diff --git a/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts b/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts index 791d5a61..105ecdaf 100644 --- a/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts +++ b/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts @@ -269,6 +269,44 @@ describe('buildChatMarkdown (server) — structure', () => { expect(md).toContain('**⚠️ Error:** 401: Unauthorized'); }); + // #274 observability: an assistant row whose turn started with a user edit to + // the open page carries metadata.pageChanged = { title, diff }; the export + // renders the diff the agent saw, before the message body. + it('renders the persisted page-change diff block for an assistant row', () => { + const md = buildChatMarkdown({ + title: 'T', + chatId: 'c', + rows: [ + row({ + role: 'assistant', + content: 'answer', + metadata: { + pageChanged: { title: 'Doc', diff: '@@ -1 +1 @@\n-old\n+new' }, + } as never, + }), + ], + }); + expect(md).toContain( + 'The user edited this page before this turn; the diff the agent saw:', + ); + expect(md).toContain('("Doc")'); + expect(md).toContain('-old'); + expect(md).toContain('+new'); + // The diff sits before the message body (chronological: change, then reply). + expect(md.indexOf('-old')).toBeLessThan(md.indexOf('answer')); + }); + + it('does not render the page-change block when metadata.pageChanged is absent', () => { + const md = buildChatMarkdown({ + title: 'T', + chatId: 'c', + rows: [row({ role: 'assistant', content: 'answer' })], + }); + expect(md).not.toContain( + 'The user edited this page before this turn; the diff the agent saw:', + ); + }); + it('escapes embedded triple-backtick fences with a longer delimiter', () => { const md = buildChatMarkdown({ title: 'T', diff --git a/apps/server/src/core/ai-chat/chat-markdown.util.ts b/apps/server/src/core/ai-chat/chat-markdown.util.ts index ebbed474..7b69b765 100644 --- a/apps/server/src/core/ai-chat/chat-markdown.util.ts +++ b/apps/server/src/core/ai-chat/chat-markdown.util.ts @@ -63,6 +63,7 @@ const LABELS: Record< tools: Record; ranTool: (name: string) => string; stillGenerating: string; + pageEditedByUser: string; } > = { en: { @@ -83,6 +84,8 @@ const LABELS: Record< ranTool: (name) => `Ran tool ${name}`, stillGenerating: 'This message is still being generated — the export captured a partial, in-progress response.', + pageEditedByUser: + 'The user edited this page before this turn; the diff the agent saw:', }, ru: { untitled: 'Без названия', @@ -102,6 +105,8 @@ const LABELS: Record< ranTool: (name) => `Выполнил инструмент ${name}`, stillGenerating: 'Это сообщение всё ещё генерируется — экспорт захватил частичный, незавершённый ответ.', + pageEditedByUser: + 'Пользователь изменил страницу перед этим ходом; дифф, который видел агент:', }, }; @@ -208,6 +213,23 @@ function rowParts(row: AiChatMessage): ExportPart[] { : [{ type: 'text', text: row.content ?? '' }]; } +/** The persisted page-change diff the agent saw this turn (#274), when any. */ +function pageChangedOf( + row: AiChatMessage, +): { title: string; diff: string } | undefined { + const meta = (row.metadata ?? {}) as { + pageChanged?: { title?: string; diff?: string }; + }; + const pc = meta.pageChanged; + if (pc && typeof pc.diff === 'string' && pc.diff.trim().length > 0) { + return { + title: typeof pc.title === 'string' ? pc.title : '', + diff: pc.diff, + }; + } + return undefined; +} + /** * Serialize a chat to a Markdown string from its persisted rows. Source = DB * ONLY (no live client state). A row whose `status` is still 'streaming' is an @@ -266,6 +288,17 @@ export function buildChatMarkdown(args: { blocks.push(``); } + // Page-change observability (#274): show the diff the agent saw at the start + // of this turn, before its response, so the export reflects the stale-page + // warning the model received. + const pc = pageChangedOf(row); + if (pc) { + const heading = pc.title + ? `${L.pageEditedByUser} ("${pc.title}")` + : L.pageEditedByUser; + blocks.push(`> **📝 ${heading}**\n\n${fence(pc.diff, 'diff')}`); + } + blocks.push(...renderMessageParts(rowParts(row), lang)); // A still-'streaming' row is an interrupted/in-progress turn captured by the -- 2.52.0 From 438ef091f966d0d8d9708952877f685ff3a4835b Mon Sep 17 00:00:00 2001 From: agent_coder Date: Thu, 2 Jul 2026 15:46:44 +0300 Subject: [PATCH 2/2] fix(#288 review): markdown-safe-escape the untrusted page title in chat export MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1: pc.title (untrusted cross-user page title) was interpolated raw into the markdown export heading. Reusing escapeAttr alone (the prompt sink's XML-attribute sanitizer, strips < > ") is insufficient here because the sink is MARKDOWN: link /image syntax survives, so a title like ![x](http://evil) or [phish](http://evil) injects a remote image / clickable link into the downloaded .md disguised as a trusted system annotation. Add markdownHeadingSafe() = escapeAttr() + backslash- escape [ and ] (disables both [text](url) and ![text](url); a bare (url) is inert). F2: cover the title branch — a title that collapses to empty via escapeAttr falls to the bare heading (no ("")), and a link/image-injection title is neutralized (non-vacuous vs the escapeAttr-only version). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/ai-chat/chat-markdown.util.spec.ts | 124 ++++++++++++++++++ .../src/core/ai-chat/chat-markdown.util.ts | 32 ++++- 2 files changed, 154 insertions(+), 2 deletions(-) diff --git a/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts b/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts index 105ecdaf..e53b5dfa 100644 --- a/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts +++ b/apps/server/src/core/ai-chat/chat-markdown.util.spec.ts @@ -307,6 +307,130 @@ describe('buildChatMarkdown (server) — structure', () => { ); }); + // #288 F1/F2: an empty page title must render the BARE heading with no + // `("…")` suffix (the `pc.title ? … : …` false branch). + it('renders the page-change heading with no title suffix when title is empty', () => { + const md = buildChatMarkdown({ + title: 'T', + chatId: 'c', + rows: [ + row({ + role: 'assistant', + content: 'answer', + metadata: { + pageChanged: { title: '', diff: '@@ -1 +1 @@\n-old\n+new' }, + } as never, + }), + ], + }); + // Bare heading, single line, no parenthesized title. + expect(md).toContain( + '> **📝 The user edited this page before this turn; the diff the agent saw:**', + ); + expect(md).not.toContain('("'); + expect(md).toContain('-old'); + }); + + // #288 F1: the page title is UNTRUSTED cross-user data, so a title carrying a + // newline / backtick / `"` / `<`/`>` must be neutralized by escapeAttr before + // it is interpolated into the `> **…**` blockquote heading — otherwise it + // could break the blockquote onto multiple lines or inject markup/HTML into + // the downloaded .md. escapeAttr strips `<>"` and collapses whitespace runs to + // a single space, so `Ev"il\n> `x` ` becomes ``Evil `x` b``. + it('escapes an untrusted page title in the page-change heading', () => { + const md = buildChatMarkdown({ + title: 'T', + chatId: 'c', + rows: [ + row({ + role: 'assistant', + content: 'answer', + metadata: { + pageChanged: { + title: 'Ev"il\n> `x` ', + diff: '@@ -1 +1 @@\n-old\n+new', + }, + } as never, + }), + ], + }); + // The heading stays a single blockquote line with the escaped title. + expect(md).toContain( + '> **📝 The user edited this page before this turn; the diff the agent saw: ("Evil `x` b")**', + ); + // No raw attribute/markup breakers survived from the title. + expect(md).not.toContain('Ev"il'); + expect(md).not.toContain(''); + }); + + // #288 review F1: escapeAttr ALONE is insufficient for this MARKDOWN sink — + // link/image syntax survives it. A cross-user title with `![x](url)` / + // `[phish](url)` must NOT become a working remote image or clickable link in + // the downloaded .md; markdownHeadingSafe backslash-escapes `[`/`]` so both are + // inert. (Non-vacuous: fails against the escapeAttr-only version, which left + // `](https://` intact.) + it('neutralizes markdown link/image syntax in an untrusted page title', () => { + const md = buildChatMarkdown({ + title: 'T', + chatId: 'c', + rows: [ + row({ + role: 'assistant', + content: 'answer', + metadata: { + pageChanged: { + title: + '![x](https://attacker.example/t.png) and [click](https://phish.example)', + diff: '@@ -1 +1 @@\n-old\n+new', + }, + } as never, + }), + ], + }); + // No WORKING image/link syntax survives — the `[…]` sits escaped as `\[…\]`, + // so the unescaped `![x](` image and `[click](` link markers are gone. (We + // deliberately do NOT assert `not.toContain('](https://')`: after escaping the + // literal `\](https://` still contains `](https://` as a raw substring — that + // check would false-fail even though the link is inert.) + expect(md).not.toContain('![x]('); + expect(md).not.toContain('[click]('); + // The brackets are backslash-escaped, so `[text](url)`/`![text](url)` are inert. + expect(md).toContain('\\['); + expect(md).toContain('\\]'); + // The heading stays a SINGLE blockquote line (no newline injected). + const headingLine = md + .split('\n') + .find((l) => l.includes('the diff the agent saw:')); + expect(headingLine).toBeDefined(); + expect(headingLine).toContain('\\[x\\]'); + expect(headingLine).toContain('\\[click\\]'); + }); + + // #288 internal review Finding 2: a NON-empty title made up entirely of + // escapeAttr breakers (`<>"`) escapes to '' — the ternary must then fall to the + // BARE heading with NO `("…")` suffix. Locks the ternary-on-escaped-value + // behavior (distinct from the empty-string input test above). + it('renders the bare heading for a title that escapes to empty', () => { + const md = buildChatMarkdown({ + title: 'T', + chatId: 'c', + rows: [ + row({ + role: 'assistant', + content: 'answer', + metadata: { + pageChanged: { title: '<>"', diff: '@@ -1 +1 @@\n-old\n+new' }, + } as never, + }), + ], + }); + expect(md).toContain( + '> **📝 The user edited this page before this turn; the diff the agent saw:**', + ); + expect(md).not.toContain('("'); + expect(md).toContain('-old'); + }); + it('escapes embedded triple-backtick fences with a longer delimiter', () => { const md = buildChatMarkdown({ title: 'T', diff --git a/apps/server/src/core/ai-chat/chat-markdown.util.ts b/apps/server/src/core/ai-chat/chat-markdown.util.ts index 7b69b765..1888f781 100644 --- a/apps/server/src/core/ai-chat/chat-markdown.util.ts +++ b/apps/server/src/core/ai-chat/chat-markdown.util.ts @@ -15,6 +15,7 @@ */ import type { AiChatMessage } from '@docmost/db/types/entity.types'; +import { escapeAttr } from './ai-chat.prompt'; /** Supported export label languages. Defaults to English. */ export type ExportLang = 'en' | 'ru'; @@ -110,6 +111,24 @@ const LABELS: Record< }, }; +/** + * Make an untrusted title safe to interpolate into a Markdown blockquote + * HEADING. escapeAttr() neutralizes the XML/HTML breakers (`<` `>` `"`) and + * collapses whitespace for the PROMPT sink (`page="…"`), but this export sink is + * MARKDOWN — link/image syntax survives escapeAttr. So additionally backslash- + * escape `[` and `]`: that disables both `[text](url)` links and `![text](url)` + * images, so a cross-user title like `![x](http://evil)` or `[phish](http://evil)` + * cannot inject a remote (auto-loading) image or a clickable link into the + * downloaded .md disguised as a trusted system annotation. A bare `(url)` with no + * preceding `[]` is inert Markdown, so brackets are the only security-critical + * characters here. (We leave backticks to escapeAttr's whitespace pass — a title + * shown as inline code cannot escape the blockquote line or load a resource, so + * it is not a security concern for this sink.) + */ +function markdownHeadingSafe(title: string): string { + return escapeAttr(title).replace(/[[\]]/g, (m) => `\\${m}`); +} + /** True for AI SDK tool parts (static `tool-*` or `dynamic-tool`). */ function isToolPart(type: string): boolean { return type.startsWith('tool-') || type === 'dynamic-tool'; @@ -293,8 +312,17 @@ export function buildChatMarkdown(args: { // warning the model received. const pc = pageChangedOf(row); if (pc) { - const heading = pc.title - ? `${L.pageEditedByUser} ("${pc.title}")` + // The page title is UNTRUSTED cross-user data (a collaborative page's title + // controllable by another user). escapeAttr() alone (the prompt sink) is + // INSUFFICIENT here: this is a MARKDOWN sink, so we neutralize link/image + // syntax too (backslash-escaping `[`/`]`) before interpolating it into this + // `> **…**` blockquote heading — otherwise `![x](url)` / `[phish](url)` would + // inject a remote image or clickable link into the downloaded .md. An + // all-`<>"` title escapes to empty and correctly falls to the bare heading. + // The diff body is already safe via fence(). (#288 review F1.) + const safeTitle = markdownHeadingSafe(pc.title); + const heading = safeTitle + ? `${L.pageEditedByUser} ("${safeTitle}")` : L.pageEditedByUser; blocks.push(`> **📝 ${heading}**\n\n${fence(pc.diff, 'diff')}`); } -- 2.52.0