[feature][ai-chat] Наблюдаемость page_changed-диффа в истории/экспорте + усиление ноты против перезаписи правок #288

Merged
vvzvlad merged 2 commits from feature/ai-chat-page-change-observability into develop 2026-07-02 19:30:56 +03:00
6 changed files with 282 additions and 8 deletions
@@ -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),
+11 -5
View File
@@ -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.
@@ -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);
});
});
/**
@@ -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,
@@ -269,6 +269,168 @@ 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:',
);
});
// #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` <b>` 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` <b>',
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('<b>');
});
// #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',
@@ -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';
@@ -63,6 +64,7 @@ const LABELS: Record<
tools: Record<string, string>;
ranTool: (name: string) => string;
stillGenerating: string;
pageEditedByUser: string;
}
> = {
en: {
@@ -83,6 +85,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,9 +106,29 @@ const LABELS: Record<
ranTool: (name) => `Выполнил инструмент ${name}`,
stillGenerating:
'Это сообщение всё ещё генерируется — экспорт захватил частичный, незавершённый ответ.',
pageEditedByUser:
'Пользователь изменил страницу перед этим ходом; дифф, который видел агент:',
},
};
/**
* 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';
@@ -208,6 +232,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 +307,26 @@ export function buildChatMarkdown(args: {
blocks.push(`<!-- ${iso} -->`);
}
// 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) {
// 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')}`);
}
blocks.push(...renderMessageParts(rowParts(row), lang));
// A still-'streaming' row is an interrupted/in-progress turn captured by the