From 2812455ba330edd4f443a31cf567a692af378214 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Mon, 15 Jun 2026 14:13:52 -0400 Subject: [PATCH 1/6] fix(policy-editor): stabilize AI Policy Assistant edit pipeline (CS-427) Addresses the consolidated policy editor / AI Policy Assistant bug cluster: - 013 artifact: strip C0/C1 control chars at the markdown boundary (new sanitizeMarkdown) so stray glyphs never reach the document. - CS-418a: preserve tableHeader cells in the content validator instead of collapsing header rows to a single empty cell. - CS-256: tie proposePolicy success to real content and show an "incomplete + Retry" card instead of a false "Policy updated" on a truncated/blank run. - CS-265: fix the list-item position off-by-one in buildPositionMap and unwrap list wrappers on apply so single/multi-bullet edits replace the intended item(s) without bleeding into neighbors or splitting the list; also fix a leading-blank-line trim that desynced lineToPos for list-first documents. - Hung/"Interrupted" runs: add maxOutputTokens (chat + edit-section), raise chat maxDuration, add a Retry affordance (regenerate), and an AbortController+timeout on the section-edit fetch. - SALE-65 + CS-265 formatting: consolidate the hand-rolled markdown converters into one symmetric mark-aware encoder/decoder (policy-markdown) used by the diff and apply paths, and neutralize inline-marker differences in normalizeContent so markup asymmetry can't create phantom suggestions. Adds 35 unit tests (converter round-trip, real-ProseMirror list apply, card-state, table headers). No production behavior outside the policy AI editor is touched. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../editor/components/PolicyDetails.tsx | 32 ++- .../editor/components/ai/markdown-utils.ts | 13 +- .../components/ai/policy-ai-assistant.tsx | 79 ++++++-- .../editor/hooks/use-suggestions.ts | 30 ++- .../lib/__tests__/list-edit-apply.test.ts | 124 ++++++++++++ .../lib/__tests__/policy-markdown.test.ts | 143 +++++++++++++ .../lib/__tests__/proposal-card-state.test.ts | 30 +++ .../[policyId]/editor/lib/apply-suggestion.ts | 43 ++++ .../editor/lib/build-position-map.ts | 37 ++-- .../editor/lib/compute-suggestion-ranges.ts | 14 +- .../[policyId]/editor/lib/policy-markdown.ts | 191 ++++++++++++++++++ .../editor/lib/proposal-card-state.ts | 29 +++ .../editor/lib/test-helpers/editor-schema.ts | 4 + .../[policyId]/editor/tools/policy-tools.ts | 10 +- .../app/api/policies/[policyId]/chat/route.ts | 9 +- .../policies/[policyId]/edit-section/route.ts | 6 +- .../editor/utils/validate-content.test.ts | 73 +++++++ .../editor/utils/validate-content.ts | 23 ++- 18 files changed, 817 insertions(+), 73 deletions(-) create mode 100644 apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/list-edit-apply.test.ts create mode 100644 apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/policy-markdown.test.ts create mode 100644 apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/proposal-card-state.test.ts create mode 100644 apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts create mode 100644 apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/policy-markdown.ts create mode 100644 apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/proposal-card-state.ts diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/PolicyDetails.tsx b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/PolicyDetails.tsx index 2e0feb3db3..b55f67b149 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/PolicyDetails.tsx +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/PolicyDetails.tsx @@ -57,6 +57,7 @@ import type { PolicyChatUIMessage } from '../types'; import { PolicyAiAssistant } from './ai/policy-ai-assistant'; import { useSuggestions } from '../hooks/use-suggestions'; import { buildPositionMap } from '../lib/build-position-map'; +import { sanitizeMarkdown } from '../lib/policy-markdown'; import { resolveInitialPolicyContent } from '../lib/resolve-initial-content'; import { InlineEditBubble } from './ai/inline-edit-bubble'; import { markdownToTipTapJSON } from './ai/markdown-utils'; @@ -111,7 +112,9 @@ function getLatestCompletedProposal(messages: PolicyChatUIMessage[]): LatestProp latest = { key: `${msg.id}:${index}`, - content: input.content, + // Strip control-char noise (e.g. stray "013" glyphs) before the content + // feeds the diff + apply pipeline, so both see identical clean text. + content: sanitizeMarkdown(input.content), summary: input.summary ?? 'Proposing policy changes', title: input.title ?? input.summary ?? 'Policy updates ready for your review', detail: @@ -471,6 +474,7 @@ export function PolicyContentManager({ messages, status, sendMessage: baseSendMessage, + regenerate, stop: stopChat, } = useChat({ transport: new DefaultChatTransport({ @@ -482,16 +486,28 @@ export function PolicyContentManager({ }, }); + // The AI needs the live editor content (it may hold unsaved accepted changes), + // so every request — initial and retry — re-derives it from the current doc. + const buildChatBody = useCallback( + () => ({ + currentContent: editorInstance + ? buildPositionMap(editorInstance.state.doc).markdown + : '', + }), + [editorInstance], + ); + const sendMessage = (payload: { text: string }) => { setChatErrorMessage(null); - // Send current editor content so the AI sees the latest state, - // not stale DB content (e.g. after accepting changes) - const currentContent = editorInstance - ? buildPositionMap(editorInstance.state.doc).markdown - : ''; - baseSendMessage(payload, { body: { currentContent } }); + baseSendMessage(payload, { body: buildChatBody() }); }; + // Recover from an interrupted/incomplete run by re-running the last request. + const handleRetryChat = useCallback(() => { + setChatErrorMessage(null); + regenerate({ body: buildChatBody() }); + }, [regenerate, buildChatBody]); + // ── Proposal state management ────────────────────────────────────── // Scan ALL assistant messages for the latest completed proposePolicy tool call. // Unlike before, this doesn't only check the last assistant message — it finds @@ -902,6 +918,7 @@ export function PolicyContentManager({ errorMessage={chatErrorMessage} sendMessage={sendMessage} stop={stopChat} + retry={handleRetryChat} close={() => setShowAiAssistant(false)} /> @@ -980,6 +997,7 @@ export function PolicyContentManager({ errorMessage={chatErrorMessage} sendMessage={sendMessage} stop={stopChat} + retry={handleRetryChat} close={() => setShowAiAssistant(false)} /> diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/ai/markdown-utils.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/ai/markdown-utils.ts index 6beb7eb160..e7ef04c9a9 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/ai/markdown-utils.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/ai/markdown-utils.ts @@ -1,7 +1,10 @@ import type { JSONContent } from '@tiptap/react'; +import { parseInline, sanitizeMarkdown } from '../../lib/policy-markdown'; export function markdownToTipTapJSON(markdown: string): Array { - const lines = markdown.split('\n'); + // Strip control-char noise (e.g. stray "013" glyphs) before parsing so it + // never reaches a ProseMirror text node. + const lines = sanitizeMarkdown(markdown).split('\n'); const content: Array = []; let currentList: JSONContent | null = null; let listType: 'bulletList' | 'orderedList' | null = null; @@ -28,7 +31,7 @@ export function markdownToTipTapJSON(markdown: string): Array { content.push({ type: 'heading', attrs: { level: headingMatch[1].length }, - content: [{ type: 'text', text: headingMatch[2] }], + content: parseInline(headingMatch[2]), }); continue; } @@ -45,7 +48,7 @@ export function markdownToTipTapJSON(markdown: string): Array { content: [ { type: 'paragraph', - content: [{ type: 'text', text: bulletMatch[1] }], + content: parseInline(bulletMatch[1]), }, ], }); @@ -64,7 +67,7 @@ export function markdownToTipTapJSON(markdown: string): Array { content: [ { type: 'paragraph', - content: [{ type: 'text', text: orderedMatch[1] }], + content: parseInline(orderedMatch[1]), }, ], }); @@ -78,7 +81,7 @@ export function markdownToTipTapJSON(markdown: string): Array { } content.push({ type: 'paragraph', - content: [{ type: 'text', text: trimmed }], + content: parseInline(trimmed), }); } diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/ai/policy-ai-assistant.tsx b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/ai/policy-ai-assistant.tsx index b53b6b74f1..f2b9f57ed7 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/ai/policy-ai-assistant.tsx +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/components/ai/policy-ai-assistant.tsx @@ -18,8 +18,9 @@ import { PromptInputSubmit, } from '@/components/ai-elements/prompt-input'; import { Button } from '@trycompai/design-system'; -import { Close, MagicWand } from '@trycompai/design-system/icons'; +import { Close, MagicWand, Renew } from '@trycompai/design-system/icons'; import type { ChatStatus } from 'ai'; +import { getProposalCardState } from '../../lib/proposal-card-state'; import type { PolicyChatUIMessage } from '../../types'; interface PolicyAiAssistantProps { @@ -28,6 +29,8 @@ interface PolicyAiAssistantProps { errorMessage?: string | null; sendMessage: (payload: { text: string }) => void; stop?: () => void; + /** Re-run the last request (used to recover from interrupted/incomplete runs). */ + retry?: () => void; close?: () => void; } @@ -37,6 +40,7 @@ export function PolicyAiAssistant({ errorMessage, sendMessage, stop, + retry, close, }: PolicyAiAssistantProps) { const isBusy = status === 'streaming' || status === 'submitted'; @@ -99,11 +103,21 @@ export function PolicyAiAssistant({ } if (part.type === 'tool-proposePolicy') { + // The proposed markdown lives in the tool INPUT. A + // truncated run can complete with empty content while + // the prose claims success (CS-256) — detect that here. + const proposed = + typeof part.input?.content === 'string' + ? part.input.content + : ''; + const hasContent = proposed.trim().length > 0; return ( ); } @@ -175,25 +189,28 @@ export function PolicyAiAssistant({ function PolicyToolCard({ state, stopped, + hasContent, + onRetry, }: { state: string; stopped: boolean; + hasContent: boolean; + onRetry?: () => void; }) { - const isCompleted = state === 'output-available'; - const isError = state === 'output-error'; - const isWorking = !isCompleted && !isError; + const cardState = getProposalCardState(state, stopped, hasContent); // Interrupted — streaming stopped while tool was in progress - if (isWorking && stopped) { + if (cardState === 'interrupted') { return ( -
- Interrupted -
+ ); } // Working state: same compact style as data tool cards - if (isWorking) { + if (cardState === 'working') { return (
- Something went wrong while generating updates. Please try again. -

+ + ); + } + + // Completed, but no content actually materialized — the run was truncated + // (timeout / output-token cap). Don't claim success on a blank policy (CS-256). + if (cardState === 'incomplete') { + return ( + ); } @@ -232,6 +261,30 @@ function PolicyToolCard({ ); } +/** + * Compact failure row for the proposePolicy tool — interrupted, errored, or + * truncated-with-no-content. Offers a Retry affordance so a dead run isn't a + * permanent dead-end (the previous UI showed a static "Interrupted" forever). + */ +function ToolFailureRow({ + message, + onRetry, +}: { + message: string; + onRetry?: () => void; +}) { + return ( +
+ {message} + {onRetry && ( + + )} +
+ ); +} + const TOOL_LABELS: Record = { 'tool-listVendors': { loading: 'Fetching vendors', done: 'Fetched vendors' }, 'tool-getVendor': { loading: 'Looking up vendor details', done: 'Looked up vendor details' }, diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/hooks/use-suggestions.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/hooks/use-suggestions.ts index 1e6f22effd..04633d950e 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/hooks/use-suggestions.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/hooks/use-suggestions.ts @@ -1,7 +1,7 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import type { Editor } from '@tiptap/react'; import { suggestionsPluginKey } from '@trycompai/ui/editor'; -import { markdownToTipTapJSON } from '../components/ai/markdown-utils'; +import { buildReplacementNodes } from '../lib/apply-suggestion'; import { buildPositionMap } from '../lib/build-position-map'; import { computeSuggestionRanges } from '../lib/compute-suggestion-ranges'; import type { SuggestionRange } from '../lib/suggestion-types'; @@ -262,19 +262,13 @@ export function useSuggestions({ tr.delete(range.from, range.to); } else if (range.type === 'insert') { // Insert at the end of the anchor position, not replacing it - const jsonNodes = markdownToTipTapJSON(range.proposedText); - const pmNodes = jsonNodes.map((json) => - editor.state.schema.nodeFromJSON(json), - ); + const pmNodes = buildReplacementNodes(editor.state, range.proposedText, range.to); if (pmNodes.length > 0) { tr.insert(range.to, pmNodes); } } else { // Modify: replace old content with new - const jsonNodes = markdownToTipTapJSON(range.proposedText); - const pmNodes = jsonNodes.map((json) => - editor.state.schema.nodeFromJSON(json), - ); + const pmNodes = buildReplacementNodes(editor.state, range.proposedText, range.from); if (pmNodes.length > 0) { tr.replaceWith(range.from, range.to, pmNodes); } @@ -358,18 +352,12 @@ export function useSuggestions({ if (range.type === 'delete') { tr.delete(range.from, range.to); } else if (range.type === 'insert') { - const jsonNodes = markdownToTipTapJSON(range.proposedText); - const pmNodes = jsonNodes.map((json) => - editor.state.schema.nodeFromJSON(json), - ); + const pmNodes = buildReplacementNodes(editor.state, range.proposedText, range.to); if (pmNodes.length > 0) { tr.insert(range.to, pmNodes); } } else { - const jsonNodes = markdownToTipTapJSON(range.proposedText); - const pmNodes = jsonNodes.map((json) => - editor.state.schema.nodeFromJSON(json), - ); + const pmNodes = buildReplacementNodes(editor.state, range.proposedText, range.from); if (pmNodes.length > 0) { tr.replaceWith(range.from, range.to, pmNodes); } @@ -415,6 +403,11 @@ export function useSuggestions({ ), ); + // Abort a stuck request so the range can't stay 'loading' forever. The + // edit-section route caps at 30s (maxDuration), so 35s is a safe ceiling. + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 35_000); + try { const policyId = window.location.pathname.match(/policies\/([^/]+)/)?.[1]; const res = await fetch(`/api/policies/${policyId}/edit-section`, { @@ -425,6 +418,7 @@ export function useSuggestions({ sectionText: range.proposedText, feedback, }), + signal: controller.signal, }); if (!res.ok) throw new Error('Failed to edit section'); @@ -444,6 +438,8 @@ export function useSuggestions({ r.id === id ? { ...r, decision: 'pending' as const } : r, ), ); + } finally { + clearTimeout(timeout); } }, [], diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/list-edit-apply.test.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/list-edit-apply.test.ts new file mode 100644 index 0000000000..18b26641e4 --- /dev/null +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/list-edit-apply.test.ts @@ -0,0 +1,124 @@ +import { describe, expect, it } from 'vitest'; +import { EditorState } from '@tiptap/pm/state'; +import type { Node as ProseMirrorNode } from '@tiptap/pm/model'; +import { buildPositionMap } from '../build-position-map'; +import { buildReplacementNodes } from '../apply-suggestion'; +import { schema } from '../test-helpers/editor-schema'; + +function listItem(text: string) { + return schema.node('listItem', null, [ + schema.node('paragraph', null, [schema.text(text)]), + ]); +} + +function bulletDoc(items: string[]) { + return schema.node('doc', null, [ + schema.node('bulletList', null, items.map(listItem)), + ]); +} + +// Real ProseMirror [before, after) boundaries for every listItem in the doc. +function realListItemBoundaries(doc: ProseMirrorNode): Array<{ from: number; to: number }> { + const out: Array<{ from: number; to: number }> = []; + doc.descendants((node, pos) => { + if (node.type.name === 'listItem') { + out.push({ from: pos, to: pos + node.nodeSize }); + } + }); + return out; +} + +// The list-item ranges the position map produced (lines beginning with "- "). +function mappedListItemRanges(doc: ProseMirrorNode): Array<{ from: number; to: number }> { + const map = buildPositionMap(doc); + const lines = map.markdown.split('\n'); + const out: Array<{ from: number; to: number }> = []; + lines.forEach((line, idx) => { + if (line.startsWith('- ')) { + const pos = map.lineToPos.get(idx + 1); + if (pos) out.push(pos); + } + }); + return out.sort((a, b) => a.from - b.from); +} + +describe('buildPositionMap list-item boundaries (CS-265)', () => { + it('maps list items to their exact ProseMirror node boundaries', () => { + const doc = bulletDoc(['First item', 'Second item', 'Third item']); + expect(mappedListItemRanges(doc)).toEqual(realListItemBoundaries(doc)); + }); + + it('is correct when a paragraph precedes the list', () => { + const doc = schema.node('doc', null, [ + schema.node('paragraph', null, [schema.text('Intro paragraph.')]), + schema.node('bulletList', null, [listItem('Alpha'), listItem('Beta')]), + ]); + expect(mappedListItemRanges(doc)).toEqual(realListItemBoundaries(doc)); + }); +}); + +describe('single-bullet edit does not bleed into neighbors (CS-265)', () => { + it('replaces only the targeted item, leaving siblings intact', () => { + const doc = bulletDoc(['Alpha', 'Bravo', 'Charlie']); + const ranges = mappedListItemRanges(doc); + const middle = ranges[1]!; // "Bravo" + + // Replicate applyRangeToDoc's "modify" path exactly. + const state = EditorState.create({ doc }); + const pmNodes = buildReplacementNodes(state, '- Bravo replaced', middle.from); + const tr = state.tr.replaceWith(middle.from, middle.to, pmNodes); + const newDoc = tr.doc; + + // The document must remain a single bulletList with exactly three sibling + // list items — no nesting, no fragments bleeding between bullets. + expect(newDoc.childCount).toBe(1); + const list = newDoc.child(0); + expect(list.type.name).toBe('bulletList'); + expect(list.childCount).toBe(3); + + const texts: string[] = []; + list.forEach((item) => { + expect(item.type.name).toBe('listItem'); + texts.push(item.textContent); + }); + expect(texts).toEqual(['Alpha', 'Bravo replaced', 'Charlie']); + }); + + it('replaces a multi-bullet range with the right number of clean items', () => { + const doc = bulletDoc(['Alpha', 'Bravo', 'Charlie']); + const ranges = mappedListItemRanges(doc); + const from = ranges[0]!.from; // start of Alpha + const to = ranges[1]!.to; // end of Bravo (covers two items) + + const state = EditorState.create({ doc }); + const pmNodes = buildReplacementNodes(state, '- One\n- Two', from); + const newDoc = state.tr.replaceWith(from, to, pmNodes).doc; + + expect(newDoc.childCount).toBe(1); + const list = newDoc.child(0); + expect(list.type.name).toBe('bulletList'); + const texts: string[] = []; + list.forEach((item) => texts.push(item.textContent)); + expect(texts).toEqual(['One', 'Two', 'Charlie']); + }); + + it('inserts a new bullet without splitting or nesting the list', () => { + const doc = bulletDoc(['Alpha', 'Bravo']); + const ranges = mappedListItemRanges(doc); + const afterFirst = ranges[0]!.to; // boundary between Alpha and Bravo + + const state = EditorState.create({ doc }); + const pmNodes = buildReplacementNodes(state, '- Inserted', afterFirst); + const newDoc = state.tr.insert(afterFirst, pmNodes).doc; + + expect(newDoc.childCount).toBe(1); + const list = newDoc.child(0); + expect(list.type.name).toBe('bulletList'); + const texts: string[] = []; + list.forEach((item) => { + expect(item.type.name).toBe('listItem'); + texts.push(item.textContent); + }); + expect(texts).toEqual(['Alpha', 'Inserted', 'Bravo']); + }); +}); diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/policy-markdown.test.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/policy-markdown.test.ts new file mode 100644 index 0000000000..b237e20cf5 --- /dev/null +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/policy-markdown.test.ts @@ -0,0 +1,143 @@ +import { describe, expect, it } from 'vitest'; +import { parseInline, sanitizeMarkdown, serializeInline } from '../policy-markdown'; +import { schema } from '../test-helpers/editor-schema'; + +// Build a paragraph PM node from inline JSON so we can round-trip through +// serializeInline against the real ProseMirror schema. +function paragraphFrom(inline: ReturnType) { + return schema.nodeFromJSON({ type: 'paragraph', content: inline }); +} + +function roundTrip(markdown: string): string { + return serializeInline(paragraphFrom(parseInline(markdown))); +} + +describe('sanitizeMarkdown', () => { + it('strips C0/C1 control chars including the "013" vertical tab', () => { + const vt = String.fromCharCode(0x0b); // vertical tab = octal 013 + const dc3 = String.fromCharCode(0x13); + const del = String.fromCharCode(0x7f); + expect(sanitizeMarkdown(`Access${vt}Control${dc3}Policy${del}`)).toBe( + 'AccessControlPolicy', + ); + }); + + it('preserves tabs and newlines', () => { + expect(sanitizeMarkdown('line1\nline2\tindented')).toBe('line1\nline2\tindented'); + }); + + it('normalizes CR and CRLF to LF', () => { + expect(sanitizeMarkdown('a\r\nb\rc')).toBe('a\nb\nc'); + }); + + it('handles empty input', () => { + expect(sanitizeMarkdown('')).toBe(''); + }); +}); + +describe('parseInline', () => { + it('returns a plain text node for unformatted text', () => { + expect(parseInline('Just plain text.')).toEqual([ + { type: 'text', text: 'Just plain text.' }, + ]); + }); + + it('parses bold, italic, code and links', () => { + expect(parseInline('**bold**')).toEqual([ + { type: 'text', text: 'bold', marks: [{ type: 'bold' }] }, + ]); + expect(parseInline('*italic*')).toEqual([ + { type: 'text', text: 'italic', marks: [{ type: 'italic' }] }, + ]); + expect(parseInline('`code`')).toEqual([ + { type: 'text', text: 'code', marks: [{ type: 'code' }] }, + ]); + expect(parseInline('[site](https://x.com)')).toEqual([ + { type: 'text', text: 'site', marks: [{ type: 'link', attrs: { href: 'https://x.com' } }] }, + ]); + }); + + it('parses a mark in the middle of a sentence', () => { + expect(parseInline('see the **Security** section')).toEqual([ + { type: 'text', text: 'see the ' }, + { type: 'text', text: 'Security', marks: [{ type: 'bold' }] }, + { type: 'text', text: ' section' }, + ]); + }); + + it('parses bold+italic ***text***', () => { + expect(parseInline('***strong***')).toEqual([ + { type: 'text', text: 'strong', marks: [{ type: 'bold' }, { type: 'italic' }] }, + ]); + }); + + it('does NOT treat snake_case underscores as italic', () => { + expect(parseInline('use the snake_case_name variable')).toEqual([ + { type: 'text', text: 'use the snake_case_name variable' }, + ]); + }); + + it('does NOT treat a lone asterisk as a mark', () => { + expect(parseInline('5 * 3 = 15')).toEqual([{ type: 'text', text: '5 * 3 = 15' }]); + }); + + it('collapses stray whitespace runs (SALE-65)', () => { + expect(parseInline('Retain records for')).toEqual([ + { type: 'text', text: 'Retain records for' }, + ]); + }); +}); + +describe('serializeInline', () => { + const bold = schema.marks.bold!.create(); + const italic = schema.marks.italic!.create(); + const code = schema.marks.code!.create(); + const link = (href: string) => schema.marks.link!.create({ href }); + + function para(...nodes: ReturnType[]) { + return schema.node('paragraph', null, nodes); + } + + it('emits markdown markers for marks', () => { + expect(serializeInline(para(schema.text('bold', [bold])))).toBe('**bold**'); + expect(serializeInline(para(schema.text('italic', [italic])))).toBe('*italic*'); + expect(serializeInline(para(schema.text('code', [code])))).toBe('`code`'); + expect(serializeInline(para(schema.text('site', [link('https://x.com')])))).toBe( + '[site](https://x.com)', + ); + }); + + it('emits ***text*** for bold+italic', () => { + expect(serializeInline(para(schema.text('s', [bold, italic])))).toBe('***s***'); + }); + + it('serializes a mid-sentence mark', () => { + expect( + serializeInline( + para( + schema.text('see the '), + schema.text('Security', [bold]), + schema.text(' section'), + ), + ), + ).toBe('see the **Security** section'); + }); +}); + +describe('round-trip parse <-> serialize', () => { + const cases = [ + 'Just plain text.', + 'A sentence with **bold** word.', + 'A sentence with *italic* word.', + 'Use `code` inline.', + 'See [the policy](https://example.com/p) here.', + 'A **bold** and *italic* mix.', + '***both at once***', + 'snake_case_name stays literal', + 'Plain ending after a [link](https://x.com).', + ]; + + it.each(cases)('round-trips: %s', (md) => { + expect(roundTrip(md)).toBe(md); + }); +}); diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/proposal-card-state.test.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/proposal-card-state.test.ts new file mode 100644 index 0000000000..7b96679297 --- /dev/null +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/proposal-card-state.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it } from 'vitest'; +import { getProposalCardState } from '../proposal-card-state'; + +describe('getProposalCardState', () => { + it('shows success only when completed WITH content', () => { + expect(getProposalCardState('output-available', false, true)).toBe('done'); + }); + + it('flags a completed-but-empty run as incomplete, never success (CS-256)', () => { + expect(getProposalCardState('output-available', false, false)).toBe('incomplete'); + }); + + it('flags a stopped in-progress run as interrupted', () => { + expect(getProposalCardState('input-streaming', true, false)).toBe('interrupted'); + expect(getProposalCardState('input-available', true, true)).toBe('interrupted'); + }); + + it('shows working while the tool is running and not stopped', () => { + expect(getProposalCardState('input-streaming', false, false)).toBe('working'); + }); + + it('surfaces tool errors', () => { + expect(getProposalCardState('output-error', false, true)).toBe('error'); + }); + + it('treats a completed run with content as done even if previously stopped flag set', () => { + // Completed (output-available) overrides the stopped heuristic. + expect(getProposalCardState('output-available', true, true)).toBe('done'); + }); +}); diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts new file mode 100644 index 0000000000..0103cd9d08 --- /dev/null +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts @@ -0,0 +1,43 @@ +import type { Node as ProseMirrorNode } from '@tiptap/pm/model'; +import type { EditorState } from '@tiptap/pm/state'; +import { markdownToTipTapJSON } from '../components/ai/markdown-utils'; + +/** + * Build the ProseMirror nodes to insert/replace for an accepted suggestion. + * + * When the edit lands inside a list, unwrap the list wrappers that + * markdownToTipTapJSON emits ("- text" -> bulletList) down to their listItems, + * so we replace a listItem with listItem(s) instead of dropping a whole new + * list into the parent list — which would otherwise split the list into pieces + * or nest it (CS-265). Outside a list, the nodes are used as-is. + */ +export function buildReplacementNodes( + state: EditorState, + proposedText: string, + at: number, +): ProseMirrorNode[] { + const pmNodes = markdownToTipTapJSON(proposedText).map((json) => + state.schema.nodeFromJSON(json), + ); + + // Resolve the parent at the edit site. Guarded so a malformed/out-of-range + // position can never throw mid-apply — fall back to the nodes as-is. + let parentName: string | null = null; + try { + parentName = state.doc.resolve(at).parent.type.name; + } catch { + return pmNodes; + } + if (parentName !== 'bulletList' && parentName !== 'orderedList') { + return pmNodes; + } + + return pmNodes.flatMap((node) => { + if (node.type.name !== 'bulletList' && node.type.name !== 'orderedList') { + return [node]; + } + const items: ProseMirrorNode[] = []; + node.forEach((child) => items.push(child)); + return items; + }); +} diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/build-position-map.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/build-position-map.ts index d7ff0cdf60..cfc9730f2b 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/build-position-map.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/build-position-map.ts @@ -1,4 +1,5 @@ import type { Node as ProseMirrorNode } from '@tiptap/pm/model'; +import { serializeInline } from './policy-markdown'; import type { PositionMap } from './suggestion-types'; export function buildPositionMap(doc: ProseMirrorNode): PositionMap { @@ -17,10 +18,15 @@ export function buildPositionMap(doc: ProseMirrorNode): PositionMap { const nodeFrom = offset + 1; // +1 because doc node itself takes position 0 if (node.type.name === 'bulletList' || node.type.name === 'orderedList') { + // `nodeFrom` (offset + 1) is already the position of the list's first + // child boundary, and `childOffset` is the listItem's offset within the + // list's content. The previous `+ 1` double-counted the boundary, pushing + // every range one position forward so `to` overshot into the NEXT item — + // which made single-bullet edits bleed into adjacent bullets (CS-265). node.forEach((listItem, childOffset) => { - const itemFrom = nodeFrom + 1 + childOffset; + const itemFrom = nodeFrom + childOffset; const itemTo = itemFrom + listItem.nodeSize; - const text = extractText(listItem); + const text = serializeInline(listItem); entries.push({ type: 'list-item', markdown: '- ' + text, from: itemFrom, to: itemTo }); }); return; @@ -30,18 +36,18 @@ export function buildPositionMap(doc: ProseMirrorNode): PositionMap { if (node.type.name === 'heading') { const level = (node.attrs.level as number) || 1; - const text = extractText(node); + const text = serializeInline(node); entries.push({ type: 'heading', markdown: '#'.repeat(level) + ' ' + text, from: nodeFrom, to: nodeTo }); } else if (node.type.name === 'paragraph') { - const text = extractText(node); + const text = serializeInline(node); entries.push({ type: 'paragraph', markdown: text, from: nodeFrom, to: nodeTo }); } else if (node.type.name === 'blockquote') { - const text = extractText(node); + const text = serializeInline(node); entries.push({ type: 'other', markdown: '> ' + text, from: nodeFrom, to: nodeTo }); } else if (node.type.name === 'horizontalRule') { entries.push({ type: 'other', markdown: '---', from: nodeFrom, to: nodeTo }); } else { - const text = extractText(node); + const text = serializeInline(node); if (text) { entries.push({ type: 'other', markdown: text, from: nodeFrom, to: nodeTo }); } @@ -65,8 +71,11 @@ export function buildPositionMap(doc: ProseMirrorNode): PositionMap { currentLine++; } - // Add blank line before first list item if previous wasn't a list item - if (entry.type === 'list-item' && prevType !== 'list-item') { + // Add blank line before a list group, but never when the list is the very + // first block: a leading blank line gets removed by the trailing .trim() + // below, which would shift every lineToPos key out of sync with the emitted + // markdown and mis-map edits in list-first documents. + if (entry.type === 'list-item' && prevType !== 'list-item' && prevType !== null) { markdownLines.push(''); currentLine++; } @@ -115,15 +124,3 @@ export function buildPositionMap(doc: ProseMirrorNode): PositionMap { markdown: markdownLines.join('\n').trim(), }; } - -function extractText(node: ProseMirrorNode): string { - let text = ''; - node.forEach((child) => { - if (child.isText) { - text += child.text ?? ''; - } else { - text += extractText(child); - } - }); - return text; -} diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts index 7eecd5e4da..263198daa6 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts @@ -176,15 +176,21 @@ function computeWordDiff(oldText: string, newText: string): DiffSegment[] { * Aggressively normalize text for comparison: * - Strip list markers (- , * , 1. ) * - Strip heading markers (# ## ### etc.) + * - Strip inline marks (bold/italic asterisks, code ticks; links -> text + href) * - Collapse all whitespace * - Lowercase - * This catches formatting-only diffs the AI introduces. + * This catches formatting-only diffs the AI introduces, and ensures any residual + * asymmetry between our markdown encoder and the model's output never surfaces + * as a phantom suggestion (real content/href changes still differ). */ function normalizeContent(text: string): string { return text - .replace(/^[\s]*[-*]\s+/gm, '') // strip list markers - .replace(/^[\s]*#{1,6}\s+/gm, '') // strip heading markers - .replace(/^[\s]*>\s+/gm, '') // strip blockquote markers + .replace(/^[\s]*[-*]\s+/gm, '') // strip list markers + .replace(/^[\s]*#{1,6}\s+/gm, '') // strip heading markers + .replace(/^[\s]*>\s+/gm, '') // strip blockquote markers + .replace(/\[([^\]]*)\]\(([^)]*)\)/g, '$1 $2') // links -> text + href + .replace(/`+/g, '') // inline code ticks + .replace(/\*+/g, '') // bold/italic asterisks .replace(/\s+/g, ' ') .trim() .toLowerCase(); diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/policy-markdown.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/policy-markdown.ts new file mode 100644 index 0000000000..4caebf2753 --- /dev/null +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/policy-markdown.ts @@ -0,0 +1,191 @@ +import type { Node as ProseMirrorNode } from '@tiptap/pm/model'; +import type { JSONContent } from '@tiptap/react'; + +/** + * Shared markdown helpers for the policy AI editor. + * + * Both the diff pipeline (build-position-map -> compute-suggestion-ranges) + * and the apply pipeline (markdown-utils -> ProseMirror) must agree on how + * markdown maps to document text, otherwise the diff and the applied edit + * drift apart and produce phantom ranges / misplaced edits. Keeping the + * conversion in one place is the single source of truth for that contract. + * + * Inline marks supported (encode + decode, kept symmetric): bold (**), italic + * (*), bold+italic (***), inline code (`) and links ([text](href)). Underscore + * emphasis is intentionally NOT parsed so snake_case identifiers in policy text + * are never mangled. Inline-marker differences are also neutralized in the diff + * (see normalizeContent in compute-suggestion-ranges) so any residual asymmetry + * can't surface as a phantom suggestion. + */ + +/** + * Control characters that must never reach the document. We keep TAB + * (U+0009) and LF (U+000A); CR is normalized to LF separately. Everything + * else in the C0/C1 control ranges is stripped -- including U+000B (vertical + * tab, octal 013), which the model occasionally emits and which surfaces as a + * stray "013" glyph inside policy text. + */ +const CONTROL_CHARS = /[\u0000-\u0008\u000B\u000C\u000E-\u001F\u007F-\u009F]/g; + +/** + * Strip control-character noise from AI-generated markdown before it enters + * the editor. Preserves newlines and tabs; normalizes CR/CRLF to LF so the + * line-based diff stays stable. + */ +export function sanitizeMarkdown(markdown: string): string { + if (!markdown) return markdown; + return markdown.replace(/\r\n?/g, '\n').replace(CONTROL_CHARS, ''); +} + +/** Collapse runs of spaces/tabs to a single space (SALE-65 stray whitespace). */ +function collapseSpaces(text: string): string { + return text.replace(/[ \t]{2,}/g, ' '); +} + +interface InlineMark { + type: string; + attrs?: Record; +} + +function addMark(marks: InlineMark[], mark: InlineMark): InlineMark[] { + if (marks.some((m) => m.type === mark.type)) return marks; + return [...marks, mark]; +} + +function makeTextNode(text: string, marks: InlineMark[]): JSONContent { + const isCode = marks.some((m) => m.type === 'code'); + // Code spans are literal — never collapse their whitespace. + const value = isCode ? text : collapseSpaces(text); + return marks.length > 0 + ? { type: 'text', text: value, marks: marks.map((m) => ({ ...m })) } + : { type: 'text', text: value }; +} + +/** + * Parse a single line of inline markdown into TipTap text nodes with marks. + * Recurses for nestable marks (bold/italic/link); code spans are terminal. + */ +export function parseInline(text: string): JSONContent[] { + return parseInlineWithMarks(text, []); +} + +function parseInlineWithMarks(text: string, marks: InlineMark[]): JSONContent[] { + const nodes: JSONContent[] = []; + let buffer = ''; + let i = 0; + + const flush = () => { + if (buffer) { + nodes.push(makeTextNode(buffer, marks)); + buffer = ''; + } + }; + + while (i < text.length) { + const rest = text.slice(i); + + // Link: [text](href) + const link = /^\[([^\]]*)\]\(([^)]*)\)/.exec(rest); + if (link) { + flush(); + nodes.push( + ...parseInlineWithMarks( + link[1]!, + addMark(marks, { type: 'link', attrs: { href: link[2]! } }), + ), + ); + i += link[0].length; + continue; + } + + // Inline code: `text` (terminal — no nested marks, raw content) + const code = /^`([^`]+)`/.exec(rest); + if (code) { + flush(); + nodes.push(makeTextNode(code[1]!, addMark(marks, { type: 'code' }))); + i += code[0].length; + continue; + } + + // Bold + italic: ***text*** + const boldItalic = /^\*\*\*([^]+?)\*\*\*/.exec(rest); + if (boldItalic) { + flush(); + nodes.push( + ...parseInlineWithMarks( + boldItalic[1]!, + addMark(addMark(marks, { type: 'bold' }), { type: 'italic' }), + ), + ); + i += boldItalic[0].length; + continue; + } + + // Bold: **text** + const bold = /^\*\*([^]+?)\*\*/.exec(rest); + if (bold) { + flush(); + nodes.push(...parseInlineWithMarks(bold[1]!, addMark(marks, { type: 'bold' }))); + i += bold[0].length; + continue; + } + + // Italic: *text* (single line, no nested asterisk) + const italic = /^\*([^*\n]+?)\*/.exec(rest); + if (italic) { + flush(); + nodes.push(...parseInlineWithMarks(italic[1]!, addMark(marks, { type: 'italic' }))); + i += italic[0].length; + continue; + } + + buffer += text[i]; + i += 1; + } + + flush(); + return nodes; +} + +/** + * Serialize a block node's inline content back to markdown, mirroring + * parseInline so encode/decode round-trip. Recurses into block children + * (e.g. a listItem's paragraph) and concatenates text. + */ +export function serializeInline(node: ProseMirrorNode): string { + let out = ''; + node.forEach((child) => { + if (child.isText) { + out += applyMarks(child.text ?? '', child.marks); + } else if (child.type.name === 'hardBreak') { + out += ' '; + } else { + out += serializeInline(child); + } + }); + return out; +} + +function applyMarks( + text: string, + marks: readonly { type: { name: string }; attrs: Record }[], +): string { + const names = new Set(marks.map((m) => m.type.name)); + const link = marks.find((m) => m.type.name === 'link'); + + let out = names.has('code') ? '`' + text + '`' : collapseSpaces(text); + + if (names.has('bold') && names.has('italic')) { + out = '***' + out + '***'; + } else if (names.has('bold')) { + out = '**' + out + '**'; + } else if (names.has('italic')) { + out = '*' + out + '*'; + } + + if (link && typeof link.attrs.href === 'string') { + out = '[' + out + '](' + link.attrs.href + ')'; + } + + return out; +} diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/proposal-card-state.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/proposal-card-state.ts new file mode 100644 index 0000000000..27d28956a8 --- /dev/null +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/proposal-card-state.ts @@ -0,0 +1,29 @@ +/** + * Pure decision for how the proposePolicy tool card should render, derived from + * the AI-SDK tool-part state, whether streaming stopped, and whether any policy + * content actually arrived. + * + * - 'interrupted': the run stopped before the tool finished. + * - 'working': the tool is still running. + * - 'error': the tool errored. + * - 'incomplete': the tool "completed" but no content materialized — a + * truncated run that must NOT be reported as success (CS-256). + * - 'done': completed with real content. + */ +export type ProposalCardState = 'interrupted' | 'working' | 'error' | 'incomplete' | 'done'; + +export function getProposalCardState( + toolState: string, + stopped: boolean, + hasContent: boolean, +): ProposalCardState { + const isCompleted = toolState === 'output-available'; + const isError = toolState === 'output-error'; + const isWorking = !isCompleted && !isError; + + if (isWorking && stopped) return 'interrupted'; + if (isWorking) return 'working'; + if (isError) return 'error'; + if (!hasContent) return 'incomplete'; + return 'done'; +} diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/test-helpers/editor-schema.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/test-helpers/editor-schema.ts index 188811f464..2fc56ff479 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/test-helpers/editor-schema.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/test-helpers/editor-schema.ts @@ -59,6 +59,10 @@ export const schema = new Schema({ parseDOM: [{ tag: 'em' }], toDOM: () => ['em', 0] as const, }, + code: { + parseDOM: [{ tag: 'code' }], + toDOM: () => ['code', 0] as const, + }, link: { attrs: { href: {} }, parseDOM: [ diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/tools/policy-tools.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/tools/policy-tools.ts index f77a2bfd04..cd1385f90a 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/tools/policy-tools.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/tools/policy-tools.ts @@ -66,8 +66,14 @@ export function getPolicyTools({ currentPolicyId, cookieHeader }: PolicyToolsOpt 'A very short imperative phrase that tells the user to review the updated policy content in the editor below.', ), }), - execute: async ({ summary, title, detail, reviewHint }) => ({ - success: true, + // Report success based on whether content actually materialized. When a + // response is truncated (timeout / output-token cap), the tool call can + // complete with empty/partial `content`, yet the assistant's prose still + // claims success — leaving the user with a blank policy (CS-256). Tie the + // tool's own `success` flag to real content so the UI can flag it. + execute: async ({ content, summary, title, detail, reviewHint }) => ({ + success: content.trim().length > 0, + contentLength: content.length, summary, title, detail, diff --git a/apps/app/src/app/api/policies/[policyId]/chat/route.ts b/apps/app/src/app/api/policies/[policyId]/chat/route.ts index 97798e22b3..f9a6b5dd69 100644 --- a/apps/app/src/app/api/policies/[policyId]/chat/route.ts +++ b/apps/app/src/app/api/policies/[policyId]/chat/route.ts @@ -6,7 +6,10 @@ import { headers } from 'next/headers'; import { NextResponse } from 'next/server'; import { getPolicyTools } from '../../../../(app)/[orgId]/policies/[policyId]/editor/tools/policy-tools'; -export const maxDuration = 60; +// Full-policy regenerations can be large; 60s was too tight and cut streams +// mid-tool-call, leaving runs stuck "Interrupted". Hosting plans clamp this to +// their own max, so raising it is safe (no-op where unsupported). +export const maxDuration = 300; export async function POST(req: Request, { params }: { params: Promise<{ policyId: string }> }) { try { @@ -193,6 +196,10 @@ You MUST produce the policy by starting from the text above and messages: await convertToModelMessages(cleanedMessages), toolChoice: 'auto', stopWhen: stepCountIs(5), + // The model must return the COMPLETE policy in the proposePolicy tool + // input. Without an explicit cap, large policies can be truncated mid-JSON + // (blank/partial content → CS-256). Give ample headroom for a full policy. + maxOutputTokens: 16000, tools: getPolicyTools({ currentPolicyId: policyId, cookieHeader: cookieStr }), }); diff --git a/apps/app/src/app/api/policies/[policyId]/edit-section/route.ts b/apps/app/src/app/api/policies/[policyId]/edit-section/route.ts index 998f5b8c21..443ca0a900 100644 --- a/apps/app/src/app/api/policies/[policyId]/edit-section/route.ts +++ b/apps/app/src/app/api/policies/[policyId]/edit-section/route.ts @@ -3,6 +3,7 @@ import { anthropic } from '@ai-sdk/anthropic'; import { generateText } from 'ai'; import { headers } from 'next/headers'; import { NextResponse } from 'next/server'; +import { sanitizeMarkdown } from '../../../../(app)/[orgId]/policies/[policyId]/editor/lib/policy-markdown'; export const maxDuration = 30; @@ -33,6 +34,9 @@ export async function POST(req: Request) { const result = await generateText({ model: anthropic('claude-sonnet-4-6'), + // A single section is small; cap output so a runaway generation can't hang + // the 30s request, and so we get a clean stop rather than a truncated edit. + maxOutputTokens: 4000, system: `You are a GRC policy editor. You will receive text from a policy and feedback on how to change it. Return ONLY the updated text. Rules: - Do not include explanations, preamble, or commentary — just the updated text. - If the input is a plain sentence or paragraph, return a plain sentence or paragraph. Do NOT add markdown formatting (no ##, no **, no -) unless the input already uses it. @@ -41,7 +45,7 @@ export async function POST(req: Request) { prompt: `Text to edit:\n${sectionText}\n\nInstruction: ${feedback}`, }); - return NextResponse.json({ updatedText: result.text.trim() }); + return NextResponse.json({ updatedText: sanitizeMarkdown(result.text).trim() }); } catch (error) { console.error('Edit section error:', error); return NextResponse.json({ error: 'Failed to edit section' }, { status: 500 }); diff --git a/packages/ui/src/components/editor/utils/validate-content.test.ts b/packages/ui/src/components/editor/utils/validate-content.test.ts index 6c748418e3..6ec235ba11 100644 --- a/packages/ui/src/components/editor/utils/validate-content.test.ts +++ b/packages/ui/src/components/editor/utils/validate-content.test.ts @@ -337,6 +337,79 @@ describe('validateAndFixTipTapContent', () => { }); }); + describe('table header preservation (CS-418)', () => { + it('preserves a tableHeader row instead of collapsing it to an empty cell', () => { + const content = { + type: 'doc', + content: [ + { + type: 'table', + content: [ + { + type: 'tableRow', + content: [ + { type: 'tableHeader', attrs: { colspan: 1, rowspan: 1 }, content: [{ type: 'paragraph', content: [{ type: 'text', text: 'Name' }] }] }, + { type: 'tableHeader', attrs: { colspan: 1, rowspan: 1 }, content: [{ type: 'paragraph', content: [{ type: 'text', text: 'Role' }] }] }, + ], + }, + { + type: 'tableRow', + content: [ + { type: 'tableCell', content: [{ type: 'paragraph', content: [{ type: 'text', text: 'Alice' }] }] }, + { type: 'tableCell', content: [{ type: 'paragraph', content: [{ type: 'text', text: 'Admin' }] }] }, + ], + }, + ], + }, + ], + }; + + const fixed = validateAndFixTipTapContent(content); + const table = (fixed.content as any[])[0]; + expect(table.type).toBe('table'); + + const headerRow = table.content[0]; + expect(headerRow.content).toHaveLength(2); + expect(headerRow.content[0].type).toBe('tableHeader'); + expect(headerRow.content[1].type).toBe('tableHeader'); + // Header text must survive + expect(headerRow.content[0].content[0].content[0].text).toBe('Name'); + expect(headerRow.content[1].content[0].content[0].text).toBe('Role'); + // Header attrs (colspan/rowspan/colwidth) must survive + expect(headerRow.content[0].attrs).toEqual({ colspan: 1, rowspan: 1 }); + + // Body row still works + const bodyRow = table.content[1]; + expect(bodyRow.content[0].type).toBe('tableCell'); + expect(bodyRow.content[0].content[0].content[0].text).toBe('Alice'); + }); + + it('preserves mixed header + cell within a single row', () => { + const content = { + type: 'doc', + content: [ + { + type: 'table', + content: [ + { + type: 'tableRow', + content: [ + { type: 'tableHeader', content: [{ type: 'paragraph', content: [{ type: 'text', text: 'Key' }] }] }, + { type: 'tableCell', content: [{ type: 'paragraph', content: [{ type: 'text', text: 'Value' }] }] }, + ], + }, + ], + }, + ], + }; + + const fixed = validateAndFixTipTapContent(content); + const row = (fixed.content as any[])[0].content[0]; + expect(row.content[0].type).toBe('tableHeader'); + expect(row.content[1].type).toBe('tableCell'); + }); + }); + describe('empty text node handling', () => { const strip = (s: string) => s.replace(/[\u00A0\u200B\u202F]/g, '').trim(); diff --git a/packages/ui/src/components/editor/utils/validate-content.ts b/packages/ui/src/components/editor/utils/validate-content.ts index 8626e3b372..6810223029 100644 --- a/packages/ui/src/components/editor/utils/validate-content.ts +++ b/packages/ui/src/components/editor/utils/validate-content.ts @@ -193,6 +193,8 @@ function fixNode(node: any): JSONContent | null { return fixTableRow(node); case 'tableCell': return fixTableCell(node); + case 'tableHeader': + return fixTableHeader(node); case 'text': return fixTextNode(node); case 'heading': @@ -408,7 +410,14 @@ function fixTableRow(node: any): JSONContent { let cells: JSONContent[] = []; if (Array.isArray(content)) { cells = content - .map((child: any) => (child?.type === 'tableCell' ? fixTableCell(child) : null)) + .map((child: any) => { + // Preserve header cells. Previously only `tableCell` was handled, so + // pasted/AI-generated header rows (`tableHeader`, e.g. from ) were + // dropped and an all-header row collapsed to a single empty cell. + if (child?.type === 'tableHeader') return fixTableHeader(child); + if (child?.type === 'tableCell') return fixTableCell(child); + return null; + }) .filter(Boolean) as JSONContent[]; } if (cells.length === 0) { @@ -417,7 +426,7 @@ function fixTableRow(node: any): JSONContent { return { type: 'tableRow', content: cells, ...(attrs && { attrs }), ...rest }; } -function fixTableCell(node: any): JSONContent { +function fixTableCellLike(node: any, type: 'tableCell' | 'tableHeader'): JSONContent { const { content, attrs, ...rest } = node; let blocks: JSONContent[] = []; if (Array.isArray(content)) { @@ -426,7 +435,15 @@ function fixTableCell(node: any): JSONContent { if (blocks.length === 0) { blocks = [createEmptyParagraph()]; } - return { type: 'tableCell', content: blocks, ...(attrs && { attrs }), ...rest }; + return { type, content: blocks, ...(attrs && { attrs }), ...rest }; +} + +function fixTableCell(node: any): JSONContent { + return fixTableCellLike(node, 'tableCell'); +} + +function fixTableHeader(node: any): JSONContent { + return fixTableCellLike(node, 'tableHeader'); } /** From 750edcd211c650e0fdb2073b8469d22f8aac00ce Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Mon, 15 Jun 2026 15:20:50 -0400 Subject: [PATCH 2/6] fix(policy-editor): don't swallow inline formatting-only edits The previous commit added inline-marker stripping to normalizeContent as a phantom-diff safety net, but it backfired: a user asking only to bold/italic/ code a phrase (same words) normalized to "no change", so zero suggestions were produced and the formatting was silently dropped despite the assistant reporting success. The markdown encoder is already mark-aware and symmetric with the model's output, so unchanged formatted content compares equal without stripping. Remove the inline-marker strip (keep block-marker + whitespace normalization) so explicit formatting edits surface as accept-able suggestions and apply as real marks. Adds regression tests for bold-only and link-only changes. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../compute-suggestion-ranges.test.ts | 29 +++++++++++++++++++ .../editor/lib/compute-suggestion-ranges.ts | 28 +++++++++--------- .../[policyId]/editor/lib/policy-markdown.ts | 7 +++-- 3 files changed, 46 insertions(+), 18 deletions(-) diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts index 520c24d5a5..5400197188 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts @@ -378,4 +378,33 @@ describe('computeSuggestionRanges', () => { expect(modRange!.proposedText).toContain('must'); }); }); + + describe('inline formatting-only changes still surface (CS-265 fmt)', () => { + it('produces a suggestion when only inline bold is added', () => { + const current = + 'Provide authentication that resists brute-force and credential-stuffing attacks.'; + const proposed = + 'Provide authentication that resists **brute-force** and **credential-stuffing** attacks.'; + const posMap = makePositionMap(current); + + const ranges = computeSuggestionRanges(posMap, proposed); + + // A user explicitly asking to bold text must get an accept-able suggestion + // — the formatting must NOT be silently swallowed by normalization. + expect(ranges.length).toBeGreaterThan(0); + expect(ranges[0]!.proposedText).toContain('**brute-force**'); + }); + + it('produces a suggestion when only a link is added', () => { + const current = 'Store passwords in an approved password manager.'; + const proposed = + 'Store passwords in an approved [password manager](https://1password.com).'; + const posMap = makePositionMap(current); + + const ranges = computeSuggestionRanges(posMap, proposed); + + expect(ranges.length).toBeGreaterThan(0); + expect(ranges[0]!.proposedText).toContain('](https://1password.com)'); + }); + }); }); diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts index 263198daa6..ec8e8a599a 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts @@ -173,24 +173,22 @@ function computeWordDiff(oldText: string, newText: string): DiffSegment[] { } /** - * Aggressively normalize text for comparison: - * - Strip list markers (- , * , 1. ) - * - Strip heading markers (# ## ### etc.) - * - Strip inline marks (bold/italic asterisks, code ticks; links -> text + href) - * - Collapse all whitespace - * - Lowercase - * This catches formatting-only diffs the AI introduces, and ensures any residual - * asymmetry between our markdown encoder and the model's output never surfaces - * as a phantom suggestion (real content/href changes still differ). + * Normalize text for comparison: + * - Strip block markers (list - / * , heading #, blockquote >) so the AI + * reformatting a block marker isn't treated as a content change. + * - Collapse all whitespace + lowercase. + * + * NOTE: inline marks (bold/italic/code/link) are deliberately NOT stripped. + * The markdown encoder is mark-aware and symmetric with the model's output, so + * unchanged formatted content already compares equal — while a user explicitly + * adding/removing formatting (e.g. "make this bold") still produces an + * accept-able suggestion instead of being silently swallowed (CS-265). */ function normalizeContent(text: string): string { return text - .replace(/^[\s]*[-*]\s+/gm, '') // strip list markers - .replace(/^[\s]*#{1,6}\s+/gm, '') // strip heading markers - .replace(/^[\s]*>\s+/gm, '') // strip blockquote markers - .replace(/\[([^\]]*)\]\(([^)]*)\)/g, '$1 $2') // links -> text + href - .replace(/`+/g, '') // inline code ticks - .replace(/\*+/g, '') // bold/italic asterisks + .replace(/^[\s]*[-*]\s+/gm, '') // strip list markers + .replace(/^[\s]*#{1,6}\s+/gm, '') // strip heading markers + .replace(/^[\s]*>\s+/gm, '') // strip blockquote markers .replace(/\s+/g, ' ') .trim() .toLowerCase(); diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/policy-markdown.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/policy-markdown.ts index 4caebf2753..d1f569ae07 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/policy-markdown.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/policy-markdown.ts @@ -13,9 +13,10 @@ import type { JSONContent } from '@tiptap/react'; * Inline marks supported (encode + decode, kept symmetric): bold (**), italic * (*), bold+italic (***), inline code (`) and links ([text](href)). Underscore * emphasis is intentionally NOT parsed so snake_case identifiers in policy text - * are never mangled. Inline-marker differences are also neutralized in the diff - * (see normalizeContent in compute-suggestion-ranges) so any residual asymmetry - * can't surface as a phantom suggestion. + * are never mangled. Because encode and decode use the same conventions, + * unchanged formatted content round-trips identically and the line diff sees no + * change — while a real formatting edit (e.g. adding bold) still diffs and can + * be accepted. */ /** From 7e6d073acb5a6500fd083dcbad96babae439c936 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Mon, 15 Jun 2026 15:30:13 -0400 Subject: [PATCH 3/6] fix(policy-editor): correct insert position, paragraph modify, and multi-edit merge Found via a new deterministic end-to-end pipeline harness (start doc + AI markdown -> positionMap -> diff -> apply against real ProseMirror), which reproduces "accept all" without a deploy. - Insert position: jsdiff's oldStart for an insertion is the line to insert BEFORE; resolveHunkPositions now collapses inserts to a zero-width point at that boundary (was inserting AFTER the anchor, so a new bullet landed below its intended neighbor). Exposed by the list-first lineToPos alignment fix. - Paragraph/heading modify: replacing a textblock's content range with a whole block node split the block and left an empty paragraph. buildReplacementNodes now splices inline content for same-shape textblock edits. - Multi-edit data loss: mergeOverlappingRanges merged two modifies across a gap, dropping the unchanged content between them (e.g. a heading) from the merged proposedText -> it was deleted on apply. Now only overlapping ranges and adjacent section-deletes merge. Adds a 17-scenario e2e harness (single/multi bullet, ordered lists, inserts, deletes, inline marks, multi-section edits, full-policy generation) as a permanent regression guard. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../compute-suggestion-ranges.test.ts | 25 +- .../editor/lib/__tests__/pipeline-e2e.test.ts | 387 ++++++++++++++++++ .../[policyId]/editor/lib/apply-suggestion.ts | 40 +- .../editor/lib/compute-suggestion-ranges.ts | 22 +- 4 files changed, 440 insertions(+), 34 deletions(-) create mode 100644 apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts index 5400197188..025a8a6475 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts @@ -262,8 +262,8 @@ describe('computeSuggestionRanges', () => { expect(allOriginalText).toContain('Section B'); }); - it('does not merge distant ranges', () => { - // Manually create a position map with widely separated positions + it('never merges separate modifies across a gap (no content loss)', () => { + // Four separate single-line changes, each separated by unchanged context. const lineToPos = new Map(); lineToPos.set(1, { from: 1, to: 10 }); lineToPos.set(3, { from: 12, to: 25 }); @@ -279,19 +279,14 @@ describe('computeSuggestionRanges', () => { const ranges = computeSuggestionRanges(posMap, proposed); - // The first two changes (positions 1-25) are close and may merge. - // The last two changes (positions 200-230) are close and may merge. - // But the two groups (25 vs 200) are >20 apart so should NOT merge. - if (ranges.length > 1) { - // Verify there's a gap between groups - const sorted = [...ranges].sort((a, b) => a.from - b.from); - for (let i = 1; i < sorted.length; i++) { - const gap = sorted[i]!.from - sorted[i - 1]!.to; - // If they didn't merge, the gap must be > 20 - if (gap > 0) { - expect(gap).toBeGreaterThan(20); - } - } + // Modifies are no longer merged across a gap (even a small one), so each + // change stays its own suggestion and no unchanged content between them + // can be swallowed/deleted on apply. + expect(ranges.length).toBe(4); + const sorted = [...ranges].sort((a, b) => a.from - b.from); + for (let i = 1; i < sorted.length; i++) { + // Non-overlapping, ascending — distinct ranges. + expect(sorted[i]!.from).toBeGreaterThanOrEqual(sorted[i - 1]!.to); } }); diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts new file mode 100644 index 0000000000..76f50173fc --- /dev/null +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts @@ -0,0 +1,387 @@ +import { describe, expect, it } from 'vitest'; +import { EditorState } from '@tiptap/pm/state'; +import type { Node as ProseMirrorNode } from '@tiptap/pm/model'; +import { buildPositionMap } from '../build-position-map'; +import { computeSuggestionRanges } from '../compute-suggestion-ranges'; +import { buildReplacementNodes } from '../apply-suggestion'; +import { sanitizeMarkdown } from '../policy-markdown'; +import { schema } from '../test-helpers/editor-schema'; + +/** + * Faithfully reproduces the real client flow for "accept all suggestions": + * proposed (sanitized) -> buildPositionMap -> computeSuggestionRanges + * -> buildReplacementNodes -> apply (reverse doc order, one transaction). + * + * This is the deterministic part of the AI edit pipeline (everything except the + * model call + React state), so it catches end-to-end logic regressions without + * a deploy. + */ +function acceptAll(startDoc: ProseMirrorNode, rawProposed: string) { + const proposed = sanitizeMarkdown(rawProposed); + const positionMap = buildPositionMap(startDoc); + const ranges = computeSuggestionRanges(positionMap, proposed); + + const state = EditorState.create({ doc: startDoc }); + const tr = state.tr; + // Reverse doc order so earlier edits don't shift later positions (matches the + // real acceptAll which resolves positions against the original doc). + for (const range of [...ranges].sort((a, b) => b.from - a.from)) { + if (range.type === 'delete') { + tr.delete(range.from, range.to); + } else if (range.type === 'insert') { + const nodes = buildReplacementNodes(state, range.proposedText, range.to); + if (nodes.length) tr.insert(range.to, nodes); + } else { + const nodes = buildReplacementNodes(state, range.proposedText, range.from); + if (nodes.length) tr.replaceWith(range.from, range.to, nodes); + } + } + return { ranges, doc: tr.doc }; +} + +// ── doc builders ── +function p(text: string) { + return schema.node('paragraph', null, text ? [schema.text(text)] : []); +} +function h(level: number, text: string) { + return schema.node('heading', { level }, [schema.text(text)]); +} +function li(text: string) { + return schema.node('listItem', null, [p(text)]); +} +function ul(...items: string[]) { + return schema.node('bulletList', null, items.map(li)); +} +function ol(...items: string[]) { + return schema.node('orderedList', null, items.map(li)); +} +function doc(...blocks: ProseMirrorNode[]) { + return schema.node('doc', null, blocks); +} + +// ── inspectors ── +function listTexts(node: ProseMirrorNode, listType = 'bulletList'): string[] { + const out: string[] = []; + node.descendants((n) => { + if (n.type.name === listType) { + n.forEach((item) => out.push(item.textContent)); + return false; + } + return true; + }); + return out; +} + +function marksOnPhrase(node: ProseMirrorNode, phrase: string): string[] { + let found: string[] | null = null; + node.descendants((n) => { + if (found) return false; + if (n.isText && n.text === phrase) { + found = n.marks.map((m) => m.type.name).sort(); + } + return true; + }); + return found ?? []; +} + +function topLevelTypes(d: ProseMirrorNode): string[] { + const out: string[] = []; + d.forEach((n) => out.push(n.type.name)); + return out; +} + +describe('E2E pipeline: single-bullet edit (CS-265)', () => { + it('changes only the targeted bullet, keeps one list', () => { + const start = doc( + h(2, 'Password Requirements'), + ul( + 'Minimum 12 characters or a passphrase; prohibit commonly breached strings.', + 'No forced periodic rotation unless compromise is suspected.', + 'Unique password per system; store passwords in an approved password manager.', + 'Do not reuse passwords across work and personal accounts.', + ), + p('Enforce length and reuse rules in IdP/directory policies and document settings.'), + ); + const proposed = [ + '## Password Requirements', + '', + '- Minimum 12 characters or a passphrase; prohibit commonly breached strings.', + '- No forced periodic rotation unless compromise is suspected.', + '- Use a unique password for every system and store it in an approved password manager.', + '- Do not reuse passwords across work and personal accounts.', + '', + 'Enforce length and reuse rules in IdP/directory policies and document settings.', + ].join('\n'); + + const { doc: result } = acceptAll(start, proposed); + expect(listTexts(result)).toEqual([ + 'Minimum 12 characters or a passphrase; prohibit commonly breached strings.', + 'No forced periodic rotation unless compromise is suspected.', + 'Use a unique password for every system and store it in an approved password manager.', + 'Do not reuse passwords across work and personal accounts.', + ]); + // Single list, trailing paragraph preserved + expect(topLevelTypes(result)).toEqual(['heading', 'bulletList', 'paragraph']); + expect(result.textContent).toContain('Enforce length and reuse rules'); + }); +}); + +describe('E2E pipeline: inline formatting (CS-265 fmt / SALE-65)', () => { + it('applies bold as real marks, not literal asterisks', () => { + const start = doc( + h(2, 'Purpose'), + p('Provide authentication that resists brute-force and credential-stuffing attacks.'), + ); + const proposed = [ + '## Purpose', + '', + 'Provide authentication that resists **brute-force** and **credential-stuffing** attacks.', + ].join('\n'); + + const { ranges, doc: result } = acceptAll(start, proposed); + expect(ranges.length).toBeGreaterThan(0); // a suggestion must appear + expect(marksOnPhrase(result, 'brute-force')).toEqual(['bold']); + expect(marksOnPhrase(result, 'credential-stuffing')).toEqual(['bold']); + expect(result.textContent).not.toContain('**'); // no literal markers + }); + + it('applies a link as a real mark', () => { + const start = doc(p('Store passwords in an approved password manager.')); + const proposed = 'Store passwords in an approved [password manager](https://1password.com).'; + const { doc: result } = acceptAll(start, proposed); + expect(marksOnPhrase(result, 'password manager')).toEqual(['link']); + expect(result.textContent).not.toContain(']('); + }); + + it('preserves existing bold when an unrelated paragraph changes', () => { + const start = doc( + schema.node('paragraph', null, [ + schema.text('Resists '), + schema.text('brute-force', [schema.marks.bold!.create()]), + schema.text(' attacks.'), + ]), + p('Scope covers all accounts.'), + ); + const proposed = [ + 'Resists **brute-force** attacks.', + '', + 'Scope covers all human and service accounts.', + ].join('\n'); + const { doc: result } = acceptAll(start, proposed); + // bold survives, only the scope sentence changed + expect(marksOnPhrase(result, 'brute-force')).toEqual(['bold']); + expect(result.textContent).toContain('all human and service accounts'); + }); +}); + +describe('E2E pipeline: list inserts', () => { + it('adds a new bullet without splitting the list', () => { + const start = doc(ul('Alpha', 'Bravo')); + const proposed = ['- Alpha', '- Inserted', '- Bravo'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(topLevelTypes(result)).toEqual(['bulletList']); + expect(listTexts(result)).toEqual(['Alpha', 'Inserted', 'Bravo']); + }); +}); + +describe('E2E pipeline: control-char sanitization (013)', () => { + it('strips control chars from applied content', () => { + const start = doc(p('Old text.')); + const vt = String.fromCharCode(0x0b); + const proposed = `New ${vt}clean${vt} text.`; + const { doc: result } = acceptAll(start, proposed); + expect(result.textContent).toBe('New clean text.'); + }); +}); + +describe('E2E pipeline: ordered list item edit', () => { + it('keeps the list ORDERED after editing an item', () => { + const start = doc(ol('First step', 'Second step', 'Third step')); + // buildPositionMap emits "- " for ordered items too, so the AI echoes "- ". + const proposed = ['- First step', '- Second step revised', '- Third step'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(topLevelTypes(result)).toEqual(['orderedList']); + expect(listTexts(result, 'orderedList')).toEqual([ + 'First step', + 'Second step revised', + 'Third step', + ]); + }); + + it('inserts a new step into an ordered list, kept ordered', () => { + const start = doc(ol('First step', 'Second step')); + const proposed = ['- First step', '- Inserted step', '- Second step'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(topLevelTypes(result)).toEqual(['orderedList']); + expect(listTexts(result, 'orderedList')).toEqual([ + 'First step', + 'Inserted step', + 'Second step', + ]); + }); +}); + +describe('E2E pipeline: delete a single bullet', () => { + it('removes only the targeted bullet', () => { + const start = doc(ul('Keep one', 'Remove me', 'Keep two')); + const proposed = ['- Keep one', '- Keep two'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(topLevelTypes(result)).toEqual(['bulletList']); + expect(listTexts(result)).toEqual(['Keep one', 'Keep two']); + }); +}); + +describe('E2E pipeline: plain paragraph modify', () => { + it('replaces paragraph text in place', () => { + const start = doc(h(2, 'Purpose'), p('Old purpose statement.')); + const proposed = ['## Purpose', '', 'New, clearer purpose statement.'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(topLevelTypes(result)).toEqual(['heading', 'paragraph']); + expect(result.textContent).toContain('New, clearer purpose statement.'); + expect(result.textContent).not.toContain('Old purpose'); + }); +}); + +describe('E2E pipeline: multi-change in one proposal', () => { + it('applies a bold edit AND an unrelated paragraph edit together', () => { + const start = doc( + h(2, 'Purpose'), + p('Provide authentication that resists brute-force attacks.'), + h(2, 'Scope'), + p('Applies to all accounts.'), + ); + const proposed = [ + '## Purpose', + '', + 'Provide authentication that resists **brute-force** attacks.', + '', + '## Scope', + '', + 'Applies to all human and service accounts.', + ].join('\n'); + const { ranges, doc: result } = acceptAll(start, proposed); + expect(ranges.length).toBeGreaterThanOrEqual(2); + expect(marksOnPhrase(result, 'brute-force')).toEqual(['bold']); + expect(result.textContent).toContain('all human and service accounts'); + expect(topLevelTypes(result)).toEqual(['heading', 'paragraph', 'heading', 'paragraph']); + }); +}); + +describe('E2E pipeline: realistic policy with two lists', () => { + it('edits one bullet in the MFA list, leaving Password Requirements intact', () => { + const start = doc( + h(2, 'Password Requirements'), + ul('Minimum 12 characters.', 'No forced rotation.', 'Unique per system.'), + h(2, 'Multi-Factor Authentication (MFA)'), + ul('Enforce MFA for admins.', 'Prefer authenticator apps; SMS only as fallback.', 'Apply step-up MFA for sensitive actions.'), + ); + const proposed = [ + '## Password Requirements', + '', + '- Minimum 12 characters.', + '- No forced rotation.', + '- Unique per system.', + '', + '## Multi-Factor Authentication (MFA)', + '', + '- Enforce MFA for admins.', + '- Prefer hardware tokens or authenticator apps; SMS only as a last resort.', + '- Apply step-up MFA for sensitive actions.', + ].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(topLevelTypes(result)).toEqual(['heading', 'bulletList', 'heading', 'bulletList']); + + const lists: string[][] = []; + result.forEach((n) => { + if (n.type.name === 'bulletList') { + const items: string[] = []; + n.forEach((it) => items.push(it.textContent)); + lists.push(items); + } + }); + expect(lists[0]).toEqual(['Minimum 12 characters.', 'No forced rotation.', 'Unique per system.']); + expect(lists[1]).toEqual([ + 'Enforce MFA for admins.', + 'Prefer hardware tokens or authenticator apps; SMS only as a last resort.', + 'Apply step-up MFA for sensitive actions.', + ]); + }); +}); + +describe('E2E pipeline: append a new section', () => { + it('adds a new heading + paragraph at the end', () => { + const start = doc(h(2, 'Purpose'), p('Purpose text.')); + const proposed = [ + '## Purpose', + '', + 'Purpose text.', + '', + '## Enforcement', + '', + 'Violations may result in disciplinary action.', + ].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(result.textContent).toContain('Purpose text.'); + expect(result.textContent).toContain('Enforcement'); + expect(result.textContent).toContain('Violations may result in disciplinary action.'); + }); +}); + +describe('E2E pipeline: bold inside a list item', () => { + it('adds bold to one bullet, keeps the list intact', () => { + const start = doc(ul('Use MFA everywhere.', 'Rotate keys quarterly.')); + const proposed = ['- Use **MFA** everywhere.', '- Rotate keys quarterly.'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(topLevelTypes(result)).toEqual(['bulletList']); + expect(listTexts(result)).toEqual(['Use MFA everywhere.', 'Rotate keys quarterly.']); + expect(marksOnPhrase(result, 'MFA')).toEqual(['bold']); + expect(result.textContent).not.toContain('**'); + }); +}); + +describe('E2E pipeline: heading text change', () => { + it('changes heading text without duplicating the heading', () => { + const start = doc(h(2, 'Password Requirements'), p('Body.')); + const proposed = ['## Password & Credential Requirements', '', 'Body.'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(topLevelTypes(result)).toEqual(['heading', 'paragraph']); + expect(result.child(0).textContent).toBe('Password & Credential Requirements'); + }); +}); + +describe('E2E pipeline: combined bold+italic', () => { + it('applies ***text*** as both marks', () => { + const start = doc(p('This is important text.')); + const proposed = 'This is ***important*** text.'; + const { doc: result } = acceptAll(start, proposed); + expect(marksOnPhrase(result, 'important')).toEqual(['bold', 'italic']); + }); +}); + +describe('E2E pipeline: generate full policy from a draft stub', () => { + it('replaces a draft stub with a full multi-section policy', () => { + const start = doc(p('This policy is a draft.')); + const proposed = [ + '## Purpose', + '', + 'Provide strong authentication.', + '', + '## Password Requirements', + '', + '- Minimum 12 characters.', + '- No forced rotation.', + '', + '## Enforcement', + '', + 'Violations may result in disciplinary action.', + ].join('\n'); + const { doc: result } = acceptAll(start, proposed); + const text = result.textContent; + expect(text).toContain('Provide strong authentication.'); + expect(text).toContain('Minimum 12 characters.'); + expect(text).toContain('Violations may result in disciplinary action.'); + expect(text).not.toContain('This policy is a draft.'); + // A real list materialized, not flattened paragraphs + expect(listTexts(result)).toEqual(['Minimum 12 characters.', 'No forced rotation.']); + }); +}); diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts index 0103cd9d08..c9c0ff20d6 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts @@ -22,22 +22,38 @@ export function buildReplacementNodes( // Resolve the parent at the edit site. Guarded so a malformed/out-of-range // position can never throw mid-apply — fall back to the nodes as-is. - let parentName: string | null = null; + let parent: ProseMirrorNode; try { - parentName = state.doc.resolve(at).parent.type.name; + parent = state.doc.resolve(at).parent; } catch { return pmNodes; } - if (parentName !== 'bulletList' && parentName !== 'orderedList') { - return pmNodes; + const parentName = parent.type.name; + + // Inside a list: unwrap the list wrappers markdownToTipTapJSON emits + // ("- text" -> bulletList) down to their listItems, so we replace a listItem + // with listItem(s) rather than dropping a whole new list into the parent list + // (which would split or nest it). CS-265. + if (parentName === 'bulletList' || parentName === 'orderedList') { + return pmNodes.flatMap((node) => { + if (node.type.name !== 'bulletList' && node.type.name !== 'orderedList') { + return [node]; + } + const items: ProseMirrorNode[] = []; + node.forEach((child) => items.push(child)); + return items; + }); + } + + // Inside a textblock (paragraph/heading): the diff range is a *content* range, + // so splice in the new block's INLINE content rather than a whole block node. + // Replacing content with a block splits the textblock and leaves an empty + // paragraph behind (CS-265). Only safe for a single same-shape block. + if (parent.isTextblock && pmNodes.length === 1 && pmNodes[0]!.isTextblock) { + const inline: ProseMirrorNode[] = []; + pmNodes[0]!.forEach((child) => inline.push(child)); + return inline; } - return pmNodes.flatMap((node) => { - if (node.type.name !== 'bulletList' && node.type.name !== 'orderedList') { - return [node]; - } - const items: ProseMirrorNode[] = []; - node.forEach((child) => items.push(child)); - return items; - }); + return pmNodes; } diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts index ec8e8a599a..18f328dab7 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts @@ -87,13 +87,15 @@ function mergeOverlappingRanges(ranges: SuggestionRange[]): SuggestionRange[] { continue; } - // Merge if overlapping or adjacent (gap of ≤20 positions, which covers - // a few empty paragraphs / blank lines between hunks). - // Always merge two adjacent deletes — they're almost certainly one section. + // Merge only when it can't drop unchanged content between the ranges: + // - overlapping ranges (any type), or + // - adjacent deletes split by blank-line context (one logical section). + // Do NOT merge two modifies across a gap — the unchanged content in the gap + // (e.g. a heading between two edited paragraphs) is absent from the merged + // proposedText and would be deleted on apply. const gap = range.from - prev.to; const shouldMerge = gap <= 0 || // overlapping - (gap <= 20 && prev.type === range.type) || // same type, close together (gap <= 20 && prev.type === 'delete' && range.type === 'delete'); // adjacent deletes if (shouldMerge) { @@ -125,9 +127,15 @@ function resolveHunkPositions( lineToPos: Map, ): { from: number; to: number } | null { if (oldLines === 0) { - const anchor = lineToPos.get(oldStart) ?? lineToPos.get(oldStart - 1); - if (!anchor) return findNearestPosition(oldStart, lineToPos); - return anchor; + // Pure insertion: collapse to a zero-width point at the right boundary. + // jsdiff's oldStart is the old line the new content is inserted BEFORE, so + // anchor to that line's start; otherwise insert after the previous line. + const before = lineToPos.get(oldStart); + if (before) return { from: before.from, to: before.from }; + const after = lineToPos.get(oldStart - 1); + if (after) return { from: after.to, to: after.to }; + const near = findNearestPosition(oldStart, lineToPos); + return near ? { from: near.to, to: near.to } : null; } let from: number | null = null; From 0350a9d1592fc12102e56d4fa812f06cdb428452 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Mon, 15 Jun 2026 15:46:56 -0400 Subject: [PATCH 4/6] fix(policy-editor): use node boundaries for blocks; fix section delete Section deletes left an empty heading behind, and any non-list delete/modify left an empty block, because non-list blocks mapped to their inner content range (single-bullet edits only worked because list items already use boundaries). Map every block to its outer [before, after] boundary so accept/delete operate on the whole block. This also lets modifies replace the whole node cleanly, so the textblock inline-splice workaround is removed. Also extract the heading-led delete-section extension into a pure, tested helper (extendDeleteRangesToSections) shared by the hook and the e2e harness. Harness now covers section delete, no-op proposals, and stray-whitespace normalization (20 scenarios). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../editor/hooks/use-suggestions.ts | 43 ++----------- .../lib/__tests__/build-position-map.test.ts | 9 ++- .../editor/lib/__tests__/pipeline-e2e.test.ts | 62 ++++++++++++++++++- .../[policyId]/editor/lib/apply-suggestion.ts | 53 +++++++++++++--- .../editor/lib/build-position-map.ts | 16 +++-- 5 files changed, 123 insertions(+), 60 deletions(-) diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/hooks/use-suggestions.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/hooks/use-suggestions.ts index 04633d950e..de2cddb75d 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/hooks/use-suggestions.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/hooks/use-suggestions.ts @@ -1,7 +1,7 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import type { Editor } from '@tiptap/react'; import { suggestionsPluginKey } from '@trycompai/ui/editor'; -import { buildReplacementNodes } from '../lib/apply-suggestion'; +import { buildReplacementNodes, extendDeleteRangesToSections } from '../lib/apply-suggestion'; import { buildPositionMap } from '../lib/build-position-map'; import { computeSuggestionRanges } from '../lib/compute-suggestion-ranges'; import type { SuggestionRange } from '../lib/suggestion-types'; @@ -167,44 +167,9 @@ export function useSuggestions({ } const positionMap = buildPositionMap(editor.state.doc); const initial = computeSuggestionRanges(positionMap, proposedMarkdown); - // For delete ranges that start at a heading, extend to the next heading - // of the same or higher level. This ensures full section deletions - // include all content between headings, even if the AI left some. - const doc = editor.state.doc; - const extended = initial.map((range) => { - if (range.type !== 'delete') return range; - - // Find the first block node in the range to check if it's a heading. - // range.from may point inside a node, so resolve to find the parent. - let headingLevel: number | null = null; - doc.nodesBetween(range.from, Math.min(range.from + 5, range.to), (node) => { - if (node.type.name === 'heading' && headingLevel === null) { - headingLevel = (node.attrs as { level?: number }).level ?? 1; - } - }); - if (headingLevel === null) return range; - - // Walk forward from the end of the range to find the next heading - // of the same or higher level. - let nextHeadingPos: number | null = null; - doc.nodesBetween(range.to, doc.content.size, (node, pos) => { - if (nextHeadingPos !== null) return false; - if (node.type.name === 'heading') { - const level = (node.attrs as { level?: number }).level ?? 1; - if (headingLevel !== null && level <= headingLevel) { - nextHeadingPos = pos; - return false; - } - } - return true; - }); - - const extendTo = nextHeadingPos ?? doc.content.size; - if (extendTo > range.to) { - return { ...range, to: extendTo }; - } - return range; - }); + // Extend heading-led delete ranges to cover the whole section so full + // section deletions include all content between headings. + const extended = extendDeleteRangesToSections(editor.state.doc, initial); setRanges(extended); setCurrentIndex(0); rangesHistoryRef.current = []; diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/build-position-map.test.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/build-position-map.test.ts index 1c9e6075aa..0399952a8f 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/build-position-map.test.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/build-position-map.test.ts @@ -68,10 +68,11 @@ describe('buildPositionMap', () => { const result = buildPositionMap(doc(h(2, 'Purpose'))); expect(result.markdown).toBe('## Purpose'); - // The heading content line should be mapped + // The heading line maps to the node's outer boundary (before the node), + // which is position 0 for the first block. const pos = result.lineToPos.get(1); expect(pos).toBeDefined(); - expect(pos!.from).toBe(1); + expect(pos!.from).toBe(0); }); it('uses the correct number of # for each level', () => { @@ -127,9 +128,11 @@ describe('buildPositionMap', () => { const result = buildPositionMap(doc(p('Hello world'))); expect(result.markdown).toBe('Hello world'); + // Maps to the paragraph's outer boundary (before the node) = 0 for the + // first block. const pos = result.lineToPos.get(1); expect(pos).toBeDefined(); - expect(pos!.from).toBe(1); + expect(pos!.from).toBe(0); }); it('separates consecutive paragraphs with a blank line', () => { diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts index 76f50173fc..0af03cb5b0 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts @@ -3,7 +3,7 @@ import { EditorState } from '@tiptap/pm/state'; import type { Node as ProseMirrorNode } from '@tiptap/pm/model'; import { buildPositionMap } from '../build-position-map'; import { computeSuggestionRanges } from '../compute-suggestion-ranges'; -import { buildReplacementNodes } from '../apply-suggestion'; +import { buildReplacementNodes, extendDeleteRangesToSections } from '../apply-suggestion'; import { sanitizeMarkdown } from '../policy-markdown'; import { schema } from '../test-helpers/editor-schema'; @@ -19,7 +19,10 @@ import { schema } from '../test-helpers/editor-schema'; function acceptAll(startDoc: ProseMirrorNode, rawProposed: string) { const proposed = sanitizeMarkdown(rawProposed); const positionMap = buildPositionMap(startDoc); - const ranges = computeSuggestionRanges(positionMap, proposed); + const ranges = extendDeleteRangesToSections( + startDoc, + computeSuggestionRanges(positionMap, proposed), + ); const state = EditorState.create({ doc: startDoc }); const tr = state.tr; @@ -358,6 +361,61 @@ describe('E2E pipeline: combined bold+italic', () => { }); }); +describe('E2E pipeline: section delete', () => { + it('removes a whole section (heading + body + list) and keeps the rest', () => { + const start = doc( + h(2, 'Purpose'), + p('Purpose body.'), + h(2, 'Deprecated Section'), + p('Outdated guidance.'), + ul('Old item one.', 'Old item two.'), + h(2, 'Enforcement'), + p('Enforcement body.'), + ); + const proposed = [ + '## Purpose', + '', + 'Purpose body.', + '', + '## Enforcement', + '', + 'Enforcement body.', + ].join('\n'); + const { doc: result } = acceptAll(start, proposed); + const text = result.textContent; + expect(text).toContain('Purpose body.'); + expect(text).toContain('Enforcement body.'); + expect(text).not.toContain('Deprecated Section'); + expect(text).not.toContain('Outdated guidance.'); + expect(text).not.toContain('Old item one.'); + // Surviving headings intact + const headings: string[] = []; + result.forEach((n) => { + if (n.type.name === 'heading') headings.push(n.textContent); + }); + expect(headings).toEqual(['Purpose', 'Enforcement']); + }); +}); + +describe('E2E pipeline: no-op proposal', () => { + it('produces zero suggestions when content is unchanged', () => { + const start = doc(h(2, 'Purpose'), p('Stays the same.'), ul('One.', 'Two.')); + const proposed = ['## Purpose', '', 'Stays the same.', '', '- One.', '- Two.'].join('\n'); + const { ranges, doc: result } = acceptAll(start, proposed); + expect(ranges.length).toBe(0); + expect(result.textContent).toBe(start.textContent); + }); +}); + +describe('E2E pipeline: tolerant of stray whitespace from the model', () => { + it('normalizes extra spaces in a modified paragraph', () => { + const start = doc(p('Keep this tidy.')); + const proposed = 'Keep this much tidier.'; + const { doc: result } = acceptAll(start, proposed); + expect(result.textContent).toBe('Keep this much tidier.'); + }); +}); + describe('E2E pipeline: generate full policy from a draft stub', () => { it('replaces a draft stub with a full multi-section policy', () => { const start = doc(p('This policy is a draft.')); diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts index c9c0ff20d6..cc0a299f99 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/apply-suggestion.ts @@ -1,6 +1,47 @@ import type { Node as ProseMirrorNode } from '@tiptap/pm/model'; import type { EditorState } from '@tiptap/pm/state'; import { markdownToTipTapJSON } from '../components/ai/markdown-utils'; +import type { SuggestionRange } from './suggestion-types'; + +/** + * Extend delete ranges that start at a heading to cover the whole section (up to + * the next heading of the same or higher level), so "remove this section" takes + * the heading AND all its content even if the diff only flagged part of it. + */ +export function extendDeleteRangesToSections( + doc: ProseMirrorNode, + ranges: SuggestionRange[], +): SuggestionRange[] { + return ranges.map((range) => { + if (range.type !== 'delete') return range; + + // Is the start of the range a heading? + let headingLevel: number | null = null; + doc.nodesBetween(range.from, Math.min(range.from + 5, range.to), (node) => { + if (node.type.name === 'heading' && headingLevel === null) { + headingLevel = (node.attrs as { level?: number }).level ?? 1; + } + }); + if (headingLevel === null) return range; + + // Walk forward to the next heading of the same or higher level. + let nextHeadingPos: number | null = null; + doc.nodesBetween(range.to, doc.content.size, (node, pos) => { + if (nextHeadingPos !== null) return false; + if (node.type.name === 'heading') { + const level = (node.attrs as { level?: number }).level ?? 1; + if (headingLevel !== null && level <= headingLevel) { + nextHeadingPos = pos; + return false; + } + } + return true; + }); + + const extendTo = nextHeadingPos ?? doc.content.size; + return extendTo > range.to ? { ...range, to: extendTo } : range; + }); +} /** * Build the ProseMirror nodes to insert/replace for an accepted suggestion. @@ -45,15 +86,7 @@ export function buildReplacementNodes( }); } - // Inside a textblock (paragraph/heading): the diff range is a *content* range, - // so splice in the new block's INLINE content rather than a whole block node. - // Replacing content with a block splits the textblock and leaves an empty - // paragraph behind (CS-265). Only safe for a single same-shape block. - if (parent.isTextblock && pmNodes.length === 1 && pmNodes[0]!.isTextblock) { - const inline: ProseMirrorNode[] = []; - pmNodes[0]!.forEach((child) => inline.push(child)); - return inline; - } - + // Otherwise the range spans whole block boundaries (see build-position-map), + // so the block nodes replace cleanly as-is. return pmNodes; } diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/build-position-map.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/build-position-map.ts index cfc9730f2b..8986b18e57 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/build-position-map.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/build-position-map.ts @@ -32,24 +32,28 @@ export function buildPositionMap(doc: ProseMirrorNode): PositionMap { return; } - const nodeTo = nodeFrom + node.nodeSize - 2; + // Use the node's outer boundaries [before, after] so accept/delete operate + // on the whole block. Using the inner content range instead leaves an empty + // block behind on delete, and splits the block on replace (CS-265). + const nodeStart = offset; + const nodeEnd = offset + node.nodeSize; if (node.type.name === 'heading') { const level = (node.attrs.level as number) || 1; const text = serializeInline(node); - entries.push({ type: 'heading', markdown: '#'.repeat(level) + ' ' + text, from: nodeFrom, to: nodeTo }); + entries.push({ type: 'heading', markdown: '#'.repeat(level) + ' ' + text, from: nodeStart, to: nodeEnd }); } else if (node.type.name === 'paragraph') { const text = serializeInline(node); - entries.push({ type: 'paragraph', markdown: text, from: nodeFrom, to: nodeTo }); + entries.push({ type: 'paragraph', markdown: text, from: nodeStart, to: nodeEnd }); } else if (node.type.name === 'blockquote') { const text = serializeInline(node); - entries.push({ type: 'other', markdown: '> ' + text, from: nodeFrom, to: nodeTo }); + entries.push({ type: 'other', markdown: '> ' + text, from: nodeStart, to: nodeEnd }); } else if (node.type.name === 'horizontalRule') { - entries.push({ type: 'other', markdown: '---', from: nodeFrom, to: nodeTo }); + entries.push({ type: 'other', markdown: '---', from: nodeStart, to: nodeEnd }); } else { const text = serializeInline(node); if (text) { - entries.push({ type: 'other', markdown: text, from: nodeFrom, to: nodeTo }); + entries.push({ type: 'other', markdown: text, from: nodeStart, to: nodeEnd }); } } }); From 0ba2525a2cb8b35bae06733b84d0aa9c8d5897fa Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Mon, 15 Jun 2026 15:48:19 -0400 Subject: [PATCH 5/6] test(policy-editor): expand e2e harness (delete sections, multi-list, blockquote) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 scenarios — all pass except a documented known limitation: a heading-LEVEL-only change (## -> ###) or block-type change with identical text is suppressed by normalizeContent's block-marker stripping (it.skip with a note). Text edits to any block work; pure structural changes are out of scope. 26 e2e scenarios total. No production changes this round. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../editor/lib/__tests__/pipeline-e2e.test.ts | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts index 0af03cb5b0..1c92c54409 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/pipeline-e2e.test.ts @@ -416,6 +416,100 @@ describe('E2E pipeline: tolerant of stray whitespace from the model', () => { }); }); +describe('E2E pipeline: heading level change', () => { + // KNOWN LIMITATION: normalizeContent strips block markers (#, -, >), so a + // change to ONLY a heading's level (or a block-type change like heading<->para + // or bullet<->para) with identical text is treated as "no change" and yields + // no suggestion. Text edits to headings work; pure structural changes do not. + // Loosening this is a deliberate behavioral choice (more sensitivity to the + // model reformatting block markers) — left out of scope pending a decision. + it.skip('changes ## to ### (whole-node replace) — known limitation', () => { + const start = doc(h(2, 'Sub Section'), p('Body.')); + const proposed = ['### Sub Section', '', 'Body.'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(result.child(0).attrs.level).toBe(3); + }); + + it('applies a heading TEXT change (whole-node replace)', () => { + const start = doc(h(2, 'Old Heading'), p('Body.')); + const proposed = ['## New Heading', '', 'Body.'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(result.child(0).type.name).toBe('heading'); + expect(result.child(0).attrs.level).toBe(2); + expect(result.child(0).textContent).toBe('New Heading'); + }); +}); + +describe('E2E pipeline: delete first / last section', () => { + it('deletes the first section cleanly', () => { + const start = doc(h(2, 'First'), p('First body.'), h(2, 'Second'), p('Second body.')); + const proposed = ['## Second', '', 'Second body.'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + const headings: string[] = []; + result.forEach((n) => { + if (n.type.name === 'heading') headings.push(n.textContent); + }); + expect(headings).toEqual(['Second']); + expect(result.textContent).not.toContain('First'); + }); + + it('deletes the last section cleanly', () => { + const start = doc(h(2, 'First'), p('First body.'), h(2, 'Second'), p('Second body.')); + const proposed = ['## First', '', 'First body.'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + const headings: string[] = []; + result.forEach((n) => { + if (n.type.name === 'heading') headings.push(n.textContent); + }); + expect(headings).toEqual(['First']); + expect(result.textContent).not.toContain('Second'); + }); +}); + +describe('E2E pipeline: two separate lists', () => { + it('edits a bullet in the second list, leaving the first untouched', () => { + const start = doc( + ul('A1', 'A2'), + p('Divider paragraph.'), + ul('B1', 'B2'), + ); + const proposed = [ + '- A1', + '- A2', + '', + 'Divider paragraph.', + '', + '- B1', + '- B2 revised', + ].join('\n'); + const { doc: result } = acceptAll(start, proposed); + const lists: string[][] = []; + result.forEach((n) => { + if (n.type.name === 'bulletList') { + const items: string[] = []; + n.forEach((it) => items.push(it.textContent)); + lists.push(items); + } + }); + expect(lists[0]).toEqual(['A1', 'A2']); + expect(lists[1]).toEqual(['B1', 'B2 revised']); + expect(result.textContent).toContain('Divider paragraph.'); + }); +}); + +describe('E2E pipeline: blockquote edit', () => { + it('edits a blockquote in place', () => { + const start = doc( + schema.node('blockquote', null, [p('Quoted guidance.')]), + p('After.'), + ); + const proposed = ['> Updated quoted guidance.', '', 'After.'].join('\n'); + const { doc: result } = acceptAll(start, proposed); + expect(result.textContent).toContain('Updated quoted guidance.'); + expect(result.textContent).toContain('After.'); + }); +}); + describe('E2E pipeline: generate full policy from a draft stub', () => { it('replaces a draft stub with a full multi-section policy', () => { const start = doc(p('This policy is a draft.')); From 789e0a1d6c5fad1dc38303e186d4c37042dbf631 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Mon, 15 Jun 2026 16:03:07 -0400 Subject: [PATCH 6/6] fix(policy-editor): correct insert-fallback side for unmapped anchors Addresses a P1 from automated review (identified by cubic): the nearest-position fallback for pure inserts used `near.to` unconditionally, so when the nearest mapped line is AFTER the insertion anchor (e.g. an insert in a double-blank gap where both neighboring lines are unmapped), content was placed after that block instead of before it. Now the side is chosen by the nearest line's position relative to the anchor (.from when at/after, .to when before). Adds tests for both sides. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../compute-suggestion-ranges.test.ts | 24 +++++++++++++++++ .../editor/lib/compute-suggestion-ranges.ts | 27 ++++++++++++++----- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts index 025a8a6475..1529cce8e1 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/__tests__/compute-suggestion-ranges.test.ts @@ -390,6 +390,30 @@ describe('computeSuggestionRanges', () => { expect(ranges[0]!.proposedText).toContain('**brute-force**'); }); + it('inserts BEFORE the nearest mapped line when it is after the anchor', () => { + // Only line 3 is mapped; an insert anchored at the unmapped top (line 1) + // must land before that block (.from = 50), not after it (cubic P1). + const lineToPos = new Map(); + lineToPos.set(3, { from: 50, to: 60 }); + const posMap: PositionMap = { lineToPos, markdown: 'aaa\nbbb\nTarget' }; + + const ranges = computeSuggestionRanges(posMap, 'NEW\naaa\nbbb\nTarget'); + const insert = ranges.find((r) => r.type === 'insert'); + expect(insert).toBeDefined(); + expect(insert!.from).toBe(50); + }); + + it('inserts AFTER the nearest mapped line when it is before the anchor', () => { + const lineToPos = new Map(); + lineToPos.set(1, { from: 5, to: 10 }); + const posMap: PositionMap = { lineToPos, markdown: 'Target\naaa\nbbb' }; + + const ranges = computeSuggestionRanges(posMap, 'Target\naaa\nbbb\nNEW'); + const insert = ranges.find((r) => r.type === 'insert'); + expect(insert).toBeDefined(); + expect(insert!.from).toBe(10); + }); + it('produces a suggestion when only a link is added', () => { const current = 'Store passwords in an approved password manager.'; const proposed = diff --git a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts index 18f328dab7..ae88f29e2c 100644 --- a/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts +++ b/apps/app/src/app/(app)/[orgId]/policies/[policyId]/editor/lib/compute-suggestion-ranges.ts @@ -134,8 +134,15 @@ function resolveHunkPositions( if (before) return { from: before.from, to: before.from }; const after = lineToPos.get(oldStart - 1); if (after) return { from: after.to, to: after.to }; - const near = findNearestPosition(oldStart, lineToPos); - return near ? { from: near.to, to: near.to } : null; + // Fallback: anchor to the nearest mapped line, choosing the side by its + // position relative to the insertion point. If the nearest line is at/after + // oldStart, insert BEFORE it (.from); otherwise insert AFTER it (.to). + // Using .to unconditionally would drop content on the wrong side. + const near = findNearestEntry(oldStart, lineToPos); + if (!near) return null; + return near.line >= oldStart + ? { from: near.from, to: near.from } + : { from: near.to, to: near.to }; } let from: number | null = null; @@ -156,22 +163,30 @@ function resolveHunkPositions( return { from, to }; } -function findNearestPosition( +function findNearestEntry( targetLine: number, lineToPos: Map, -): { from: number; to: number } | null { - let closest: { from: number; to: number } | null = null; +): { line: number; from: number; to: number } | null { + let closest: { line: number; from: number; to: number } | null = null; let closestDist = Infinity; for (const [line, pos] of lineToPos) { const dist = Math.abs(line - targetLine); if (dist < closestDist) { closestDist = dist; - closest = pos; + closest = { line, from: pos.from, to: pos.to }; } } return closest; } +function findNearestPosition( + targetLine: number, + lineToPos: Map, +): { from: number; to: number } | null { + const entry = findNearestEntry(targetLine, lineToPos); + return entry ? { from: entry.from, to: entry.to } : null; +} + function computeWordDiff(oldText: string, newText: string): DiffSegment[] { const changes = diffWords(oldText, newText); return changes.map((change) => ({