From 1c1a3b33f0b81c05b837ee6ce2dda993d8c1b49a Mon Sep 17 00:00:00 2001 From: Steven Obiajulu Date: Mon, 28 Sep 2026 10:57:02 -0400 Subject: [PATCH 1/4] fix(docx-mcp): no-op accept/reject has nothing to persist; clean replace warns on hidden fields and note refs accept_ai_edits / reject_ai_edits called with revision_ids that matched no revision reported persistence_required: true with a save instruction, echoed the unknown ids as selected_revision_ids, and recorded a selective action that made a later clean save fail with SELECTIVE_REVISIONS_WOULD_BE_DISCARDED although nothing had changed. docx-core echoes requested ids whether or not they exist, so the tool now gates the action record, persistence_required, next_step and the echoed ids on the result counts (the same predicate #1084 used for markEdited). A real selective accept/reject still records the action and still blocks a clean save that would discard remaining AI revisions. A clean (untracked) replace_text whose range spanned a result-less complex field (an XE or TC entry) or a footnote/endnote reference removed it and returned success with no warning, while the same path already warned for a w:sym character (#1044). The tool now counts live result-less fields and note references in the paragraph before and after the edit and warns once per construct removed, naming the field instruction or the note id, in the same style as the symbol warning (tracked mode says the construct was deleted as a tracked change). Fields with a cached result are not counted: their result is in the text the caller matched. The counting stays in the MCP layer; docx-core's replace path is untouched. Fixes: #1099 Fixes: #1097 --- .../docx-mcp/src/tools/accept_ai_edits.ts | 21 +- .../add_selective_ai_accept_reject.test.ts | 116 ++++++++- .../docx-mcp/src/tools/reject_ai_edits.ts | 21 +- packages/docx-mcp/src/tools/replace_text.ts | 82 ++++++ ...idden_construct_warning.regression.test.ts | 235 ++++++++++++++++++ 5 files changed, 459 insertions(+), 16 deletions(-) create mode 100644 packages/docx-mcp/src/tools/replace_text_hidden_construct_warning.regression.test.ts diff --git a/packages/docx-mcp/src/tools/accept_ai_edits.ts b/packages/docx-mcp/src/tools/accept_ai_edits.ts index 617d33f4..8775d12c 100644 --- a/packages/docx-mcp/src/tools/accept_ai_edits.ts +++ b/packages/docx-mcp/src/tools/accept_ai_edits.ts @@ -37,21 +37,28 @@ export async function acceptAiEdits( author: params.author, normalizeFirst: params.normalize_first, }); - // A selector that resolved nothing must not look like an edit (#1084). - if (revisionResultChangedDocument(result)) manager.markEdited(session); - if (selectedIds.length > 0) { + // A selector that resolved nothing must not look like an edit (#1084), and + // a call that changed nothing has nothing to persist: it records no + // selective action (which would make a later clean save fail with + // SELECTIVE_REVISIONS_WOULD_BE_DISCARDED) and echoes no ids as selected. + // docx-core echoes requested revision_ids whether or not they exist, so + // the result counts, not the echoed ids, are what prove a change (#1099). + const changed = revisionResultChangedDocument(result); + const effectiveIds = changed ? selectedIds : []; + if (changed) { + manager.markEdited(session); manager.recordSelectiveRevisionAction(session, { tool: 'accept_ai_edits', selector: hasIds ? 'revision_ids' : 'author', - selectedRevisionIds: selectedIds, + selectedRevisionIds: effectiveIds, }); } return ok(mergeSessionResolutionMetadata({ ...result, - selected_revision_ids: selectedIds, + selected_revision_ids: effectiveIds, file_path: manager.normalizePath(session.originalPath), - persistence_required: selectedIds.length > 0, - ...(selectedIds.length > 0 + persistence_required: changed, + ...(changed ? { next_step: "Call save with save_format='tracked' or 'both' to persist this session-scoped mutation." } : {}), }, metadata)); diff --git a/packages/docx-mcp/src/tools/add_selective_ai_accept_reject.test.ts b/packages/docx-mcp/src/tools/add_selective_ai_accept_reject.test.ts index acb6f12a..a13d4352 100644 --- a/packages/docx-mcp/src/tools/add_selective_ai_accept_reject.test.ts +++ b/packages/docx-mcp/src/tools/add_selective_ai_accept_reject.test.ts @@ -331,8 +331,9 @@ describe('No-op accept/reject leaves the session unedited (#1084)', () => { assertSuccess(byAuthor, 'reject_ai_edits'); assertSuccess(byIds, 'reject_ai_edits'); expect(byAuthor.selected_revision_ids).toEqual([]); - // Unknown revision_ids are echoed back as selected, so zero counts (not an - // empty selection) are what prove nothing changed. + // Unknown revision_ids are no longer echoed back as selected (#1099); + // the zero counts are what prove nothing changed. + expect(byIds.selected_revision_ids).toEqual([]); expect(byIds.insertionsRemoved).toBe(0); expect(byIds.deletionsRestored).toBe(0); expect(counters(session)).toEqual(before); @@ -394,3 +395,114 @@ describe('No-op accept/reject leaves the session unedited (#1084)', () => { }); }); }); + +// #1099: accept/reject with revision_ids that match nothing used to report +// persistence_required: true with a save instruction, echo the unknown ids as +// selected, and record a selective action that made a later clean save fail +// with SELECTIVE_REVISIONS_WOULD_BE_DISCARDED although nothing had changed. +describe('No-op accept/reject by unknown revision id has nothing to persist (#1099)', () => { + registerCleanup(); + + const TOOLS = [ + { name: 'accept_ai_edits', call: acceptAiEdits }, + { name: 'reject_ai_edits', call: rejectAiEdits }, + ] as const; + + for (const tool of TOOLS) { + test(`${tool.name} with revision ids that match no revision reports persistence_required: false and no next_step`, async ({ given, when, then }: AllureBddContext) => { + const opened = await given('a session with AI and reviewer revisions', () => + openSession([], { mgr: manager(), xml: documentXml(MIXED_AUTHOR_BODY) }), + ); + const session = await docxSession(opened.mgr, opened.filePath); + + const result = await when(`${tool.name} targets revision ids that do not exist`, () => + tool.call(opened.mgr, { file_path: opened.filePath, revision_ids: [999, '998'] }), + ); + + await then('it succeeds, reports nothing to persist and records no selective action', () => { + assertSuccess(result, tool.name); + expect(result.persistence_required).toBe(false); + expect(result).not.toHaveProperty('next_step'); + expect(result.selected_revision_ids).toEqual([]); + expect(session.selectiveRevisionAction).toBeNull(); + }); + }); + + test(`a clean save after a no-op ${tool.name} succeeds exactly as it would without the call`, async ({ given, when, then }: AllureBddContext) => { + const opened = await given('a session with AI and reviewer revisions', () => + openSession([], { mgr: manager(), xml: documentXml(MIXED_AUTHOR_BODY) }), + ); + const control = await given('a control session on the same document with no accept/reject call', () => + openSession([], { mgr: manager(), xml: documentXml(MIXED_AUTHOR_BODY) }), + ); + const cleanPath = path.join(opened.tmpDir, 'no-op-by-id-clean.docx'); + const controlPath = path.join(control.tmpDir, 'control-clean.docx'); + + const saved = await when(`${tool.name} targets an unknown revision id and the session is saved clean`, async () => { + assertSuccess(await tool.call(opened.mgr, { file_path: opened.filePath, revision_ids: [999] }), tool.name); + return save(opened.mgr, { file_path: opened.filePath, save_to_local_path: cleanPath, save_format: 'clean' }); + }); + const controlSaved = await when('the control session is saved clean', () => + save(control.mgr, { file_path: control.filePath, save_to_local_path: controlPath, save_format: 'clean' }), + ); + + await then('both clean saves succeed and write the same document.xml', async () => { + assertSuccess(saved, 'clean save after no-op selection by id'); + assertSuccess(controlSaved, 'control clean save'); + expect(saved.selective_revision_disposition).toBeUndefined(); + expect(controlSaved.selective_revision_disposition).toBeUndefined(); + const xml = await (await DocxZip.load(await fs.readFile(cleanPath))).readText('word/document.xml'); + const controlXml = await (await DocxZip.load(await fs.readFile(controlPath))).readText('word/document.xml'); + expect(xml).toBe(controlXml); + }); + }); + } + + test('a real selective accept still records the action and blocks a clean save that would discard remaining AI revisions (control)', async ({ given, when, then }: AllureBddContext) => { + const opened = await given('a session with AI and reviewer revisions', () => + openSession([], { mgr: manager(), xml: documentXml(MIXED_AUTHOR_BODY) }), + ); + const session = await docxSession(opened.mgr, opened.filePath); + const cleanPath = path.join(opened.tmpDir, 'real-selective-clean.docx'); + + const accepted = await when('accept_ai_edits accepts one of the two AI revisions by id', () => + acceptAiEdits(opened.mgr, { file_path: opened.filePath, revision_ids: [101] }), + ); + const blocked = await when('the session is saved clean without an acknowledgement', () => + save(opened.mgr, { file_path: opened.filePath, save_to_local_path: cleanPath, save_format: 'clean' }), + ); + + await then('the call reports the selection to persist and the clean save is refused', () => { + assertSuccess(accepted, 'accept_ai_edits'); + expect(accepted.selected_revision_ids).toEqual(['101']); + expect(accepted.persistence_required).toBe(true); + expect(accepted.next_step).toMatch(/save_format='tracked'/u); + expect(session.selectiveRevisionAction).toMatchObject({ tool: 'accept_ai_edits', selector: 'revision_ids', selectedRevisionIds: ['101'] }); + assertFailure(blocked, 'SELECTIVE_REVISIONS_WOULD_BE_DISCARDED'); + }); + }); + + test('a real selective reject still records the action and blocks a clean save that would discard remaining AI revisions (control)', async ({ given, when, then }: AllureBddContext) => { + const opened = await given('a session with AI and reviewer revisions', () => + openSession([], { mgr: manager(), xml: documentXml(MIXED_AUTHOR_BODY) }), + ); + const session = await docxSession(opened.mgr, opened.filePath); + const cleanPath = path.join(opened.tmpDir, 'real-selective-reject-clean.docx'); + + const rejected = await when('reject_ai_edits rejects one of the two AI revisions by id', () => + rejectAiEdits(opened.mgr, { file_path: opened.filePath, revision_ids: [103] }), + ); + const blocked = await when('the session is saved clean without an acknowledgement', () => + save(opened.mgr, { file_path: opened.filePath, save_to_local_path: cleanPath, save_format: 'clean' }), + ); + + await then('the call reports the selection to persist and the clean save is refused', () => { + assertSuccess(rejected, 'reject_ai_edits'); + expect(rejected.selected_revision_ids).toEqual(['103']); + expect(rejected.persistence_required).toBe(true); + expect(rejected.next_step).toMatch(/save_format='tracked'/u); + expect(session.selectiveRevisionAction).toMatchObject({ tool: 'reject_ai_edits', selector: 'revision_ids', selectedRevisionIds: ['103'] }); + assertFailure(blocked, 'SELECTIVE_REVISIONS_WOULD_BE_DISCARDED'); + }); + }); +}); diff --git a/packages/docx-mcp/src/tools/reject_ai_edits.ts b/packages/docx-mcp/src/tools/reject_ai_edits.ts index f504a563..1514bbe2 100644 --- a/packages/docx-mcp/src/tools/reject_ai_edits.ts +++ b/packages/docx-mcp/src/tools/reject_ai_edits.ts @@ -38,21 +38,28 @@ export async function rejectAiEdits( author: params.author, normalizeFirst: params.normalize_first, }); - // A selector that resolved nothing must not look like an edit (#1084). - if (revisionResultChangedDocument(result)) manager.markEdited(session); - if (selectedIds.length > 0) { + // A selector that resolved nothing must not look like an edit (#1084), and + // a call that changed nothing has nothing to persist: it records no + // selective action (which would make a later clean save fail with + // SELECTIVE_REVISIONS_WOULD_BE_DISCARDED) and echoes no ids as selected. + // docx-core echoes requested revision_ids whether or not they exist, so + // the result counts, not the echoed ids, are what prove a change (#1099). + const changed = revisionResultChangedDocument(result); + const effectiveIds = changed ? selectedIds : []; + if (changed) { + manager.markEdited(session); manager.recordSelectiveRevisionAction(session, { tool: 'reject_ai_edits', selector: hasIds ? 'revision_ids' : 'author', - selectedRevisionIds: selectedIds, + selectedRevisionIds: effectiveIds, }); } return ok(mergeSessionResolutionMetadata({ ...result, - selected_revision_ids: selectedIds, + selected_revision_ids: effectiveIds, file_path: manager.normalizePath(session.originalPath), - persistence_required: selectedIds.length > 0, - ...(selectedIds.length > 0 + persistence_required: changed, + ...(changed ? { next_step: "Call save with save_format='tracked' or 'both' to persist this session-scoped mutation." } : {}), }, metadata)); diff --git a/packages/docx-mcp/src/tools/replace_text.ts b/packages/docx-mcp/src/tools/replace_text.ts index 07d8737a..15658203 100644 --- a/packages/docx-mcp/src/tools/replace_text.ts +++ b/packages/docx-mcp/src/tools/replace_text.ts @@ -56,6 +56,84 @@ function symbolRemovalWarning(removed: number, tracked: boolean): string { `not shown in the paragraph text; ${removed === 1 ? 'it was' : 'they were'} ${outcome}.`; } +function insideTrackedDeletion(el: Element, paragraph: Element): boolean { + for (let node: Node | null = el.parentNode; node && node !== paragraph; node = node.parentNode) { + if (node.nodeType === 1 && (node as Element).namespaceURI === OOXML.W_NS && (node as Element).localName === W.del) { + return true; + } + } + return false; +} + +/** + * Other constructs the paragraph text does not show and a range can take out + * without the caller having seen them (issue #1097): a field with no result + * (an `XE` index entry, a `TC` entry, a `PAGE` never updated: complex fields + * with no `separate` marker) and a footnote or endnote reference. Each is + * described once, in document order, so a before/after comparison names + * exactly what the edit removed. Fields with a cached result are left out: + * their result is in the paragraph text the caller matched, so the caller saw + * what the range covers. An empty `w:fldSimple` is a zero-length marker the + * replace primitive keeps in place, so it needs no count. + */ +function collectLiveHiddenConstructs(paragraph: Element): string[] { + const labels: string[] = []; + const openFields: Array<{ instruction: string; separated: boolean }> = []; + const all = paragraph.getElementsByTagNameNS(OOXML.W_NS, '*'); + for (let i = 0; i < all.length; i++) { + const el = all[i]!; + if (insideTrackedDeletion(el, paragraph)) continue; + switch (el.localName) { + case W.fldChar: { + const kind = el.getAttributeNS(OOXML.W_NS, 'fldCharType') ?? el.getAttribute('w:fldCharType'); + if (kind === 'begin') openFields.push({ instruction: '', separated: false }); + else if (kind === 'separate' && openFields.length > 0) openFields[openFields.length - 1]!.separated = true; + else if (kind === 'end') { + const field = openFields.pop(); + if (field && !field.separated) labels.push(`field with no result (instruction: ${field.instruction.trim()})`); + } + break; + } + case W.instrText: + if (openFields.length > 0) openFields[openFields.length - 1]!.instruction += el.textContent ?? ''; + break; + case W.footnoteReference: + case W.endnoteReference: { + const id = el.getAttributeNS(OOXML.W_NS, 'id') ?? el.getAttribute('w:id') ?? '?'; + labels.push(`${el.localName === W.footnoteReference ? 'footnote' : 'endnote'} reference (note id ${id})`); + break; + } + default: + break; + } + } + return labels; +} + +/** Labels present before the edit and missing after it, as a multiset difference. */ +function removedHiddenConstructs(before: string[], after: string[]): string[] { + const remaining = new Map(); + for (const label of after) remaining.set(label, (remaining.get(label) ?? 0) + 1); + const removed: string[] = []; + for (const label of before) { + const count = remaining.get(label) ?? 0; + if (count > 0) remaining.set(label, count - 1); + else removed.push(label); + } + return removed; +} + +function hiddenConstructRemovalWarning(label: string, tracked: boolean): string { + const outcome = tracked + ? 'deleted with the replaced text as a tracked change; reject the deletion to restore' + : 'removed with the replaced text'; + const aftermath = !tracked && /^(?:footnote|endnote) reference/u.test(label) + ? ' Its note body stays in the notes part, no longer referenced.' + : ''; + const article = /^[aeiou]/iu.test(label) ? 'an' : 'a'; + return `The replaced range spanned ${article} ${label} not shown in the paragraph text; it was ${outcome}.${aftermath}`; +} + function mergeAddRunProps( a: NonNullable | null | undefined, b: NonNullable | null | undefined, @@ -278,9 +356,13 @@ export async function replaceText( throw new Error(`Paragraph ID ${pid} not found in document`); } const liveSymbolsBefore = countLiveSymbolCharacters(pEl); + const hiddenBefore = collectLiveHiddenConstructs(pEl); const reportRemovedSymbols = (): void => { const removed = liveSymbolsBefore - countLiveSymbolCharacters(pEl); if (removed > 0) editWarnings.push(symbolRemovalWarning(removed, !!activeCtx)); + for (const label of removedHiddenConstructs(hiddenBefore, collectLiveHiddenConstructs(pEl))) { + editWarnings.push(hiddenConstructRemovalWarning(label, !!activeCtx)); + } }; const paraRuns = getParagraphRuns(pEl); diff --git a/packages/docx-mcp/src/tools/replace_text_hidden_construct_warning.regression.test.ts b/packages/docx-mcp/src/tools/replace_text_hidden_construct_warning.regression.test.ts new file mode 100644 index 00000000..0c99f124 --- /dev/null +++ b/packages/docx-mcp/src/tools/replace_text_hidden_construct_warning.regression.test.ts @@ -0,0 +1,235 @@ +/** + * Regression test for issue #1097: an untracked (clean) replace_text whose + * range spans content that is not in the paragraph text — a result-less field + * such as an `XE` index entry, or a footnote / endnote reference — removed it + * and returned success with no warning. The clean path already warned for a + * `w:sym` character (#1044) and the tracked path records the removal as a + * `w:del` (#1082); the caller of a clean edit was the only one who could not + * tell that something it never saw was deleted. + * + * Exercises the package path — open → replace_text (→ save clean) — and + * asserts the issue's acceptance criteria: the response names the field + * instruction or the note id it removed, and an edit that removes none of + * these raises no new warning. + */ +import path from 'node:path'; +import { describe, expect } from 'vitest'; +import { testAllure, type AllureBddContext } from '../testing/allure-test.js'; +import { + openSession, + assertSuccess, + registerCleanup, + createTestSessionManager, +} from '../testing/session-test-utils.js'; +import { replaceText } from './replace_text.js'; +import { save } from './save.js'; +import { readDocumentXmlFromPath } from '../testing/docx_test_utils.js'; + +const test = testAllure.epic('Document Editing').withLabels({ feature: 'Replace Text' }); + +const W_NS = 'http://schemas.openxmlformats.org/wordprocessingml/2006/main'; + +const T = (s: string): string => `${s}`; + +const XE_FIELD = + '' + + ' XE "Alpha" ' + + ''; + +const REF_FIELD_WITH_RESULT = + '' + + ' REF Sec1 \\h ' + + '' + + T('Section 1') + + ''; + +const FOOTNOTE_REF = ''; +const ENDNOTE_REF = ''; + +const FOOTNOTES_XML = + `` + + '' + + '' + + ' Note text.' + + ''; + +const ENDNOTES_XML = + `` + + '' + + '' + + ' End note text.' + + ''; + +function wrapDoc(paragraph: string): string { + return '' + + `` + + '' + + 'Intro paragraph.' + + paragraph + + 'Closing paragraph.' + + ''; +} + +async function cleanReplace( + paragraph: string, + oldString: string, + newString: string, + extraFiles?: Record, +): Promise<{ warnings: string[]; afterText: string; savedXml: string }> { + const mgr = createTestSessionManager({ defaultAiAuthor: null }); + const session = await openSession([], { mgr, xml: wrapDoc(paragraph), extraFiles }); + const replaced = await replaceText(mgr, { + file_path: session.inputPath, + target_paragraph_id: session.paraIds[1]!, + old_string: oldString, + new_string: newString, + instruction: 'edit', + }); + assertSuccess(replaced, 'replace_text'); + const outPath = path.join(session.tmpDir, 'out-clean.docx'); + assertSuccess(await save(mgr, { file_path: session.inputPath, save_to_local_path: outPath, save_format: 'clean' }), 'save clean'); + return { + warnings: (replaced as { warnings?: string[] }).warnings ?? [], + afterText: String(replaced.after_text), + savedXml: await readDocumentXmlFromPath(outPath), + }; +} + +describe('replace_text — a clean range spanning a result-less field or a note reference (#1097)', () => { + registerCleanup(); + + test('a clean replace that removes a result-less XE field warns and names the field instruction', async ({ given, when, then }: AllureBddContext) => { + const paragraph = await given('a paragraph "Alpha ⟨XE "Alpha"⟩Bravo" whose index entry is not in the paragraph text', () => + `${T('Alpha ')}${XE_FIELD}${T('Bravo')}`); + + const result = await when('replace_text replaces "Alpha Bravo" with "Charlie" without tracking', () => + cleanReplace(paragraph, 'Alpha Bravo', 'Charlie')); + + await then('the response warns that a field with instruction XE "Alpha" was removed, and the saved paragraph has no field', () => { + expect(result.afterText).toBe('Charlie'); + expect(result.warnings).toHaveLength(1); + expect(result.warnings[0]).toBe( + 'The replaced range spanned a field with no result (instruction: XE "Alpha") not shown in the paragraph text; it was removed with the replaced text.', + ); + expect(result.savedXml).not.toContain('instrText'); + }); + }); + + test('control: an empty w:fldSimple inside the range survives the clean replace and raises no warning', async ({ given, when, then }: AllureBddContext) => { + const paragraph = await given('a paragraph with an empty w:fldSimple PAGE field between two words', () => + `${T('Alpha ')}${T(' Bravo')}`); + + const result = await when('replace_text replaces the range spanning the field', () => + cleanReplace(paragraph, 'a B', 'X')); + + await then('the field marker is still in the saved paragraph and nothing is reported removed', () => { + expect(result.afterText).toBe('AlphXravo'); + expect(result.warnings).toEqual([]); + expect(result.savedXml).toContain(''); + }); + }); + + test('a clean replace that removes a footnote reference warns and names the note id', async ({ given, when, then }: AllureBddContext) => { + const paragraph = await given('a paragraph "Alpha⟨footnoteReference 1⟩ Bravo" with its footnotes part', () => + `${T('Alpha')}${FOOTNOTE_REF}${T(' Bravo')}`); + + const result = await when('replace_text replaces "Alpha Bravo" with "Charlie" without tracking', () => + cleanReplace(paragraph, 'Alpha Bravo', 'Charlie', { 'word/footnotes.xml': FOOTNOTES_XML })); + + await then('the response warns that footnote reference 1 was removed and that its body remains', () => { + expect(result.afterText).toBe('Charlie'); + expect(result.warnings).toEqual([ + 'The replaced range spanned a footnote reference (note id 1) not shown in the paragraph text; it was removed with the replaced text. Its note body stays in the notes part, no longer referenced.', + ]); + expect(result.savedXml).not.toContain('footnoteReference'); + }); + }); + + test('a clean replace that removes an endnote reference warns and names the note id', async ({ given, when, then }: AllureBddContext) => { + const paragraph = await given('a paragraph "Alpha⟨endnoteReference 1⟩ Bravo" with its endnotes part', () => + `${T('Alpha')}${ENDNOTE_REF}${T(' Bravo')}`); + + const result = await when('replace_text replaces "Alpha Bravo" with "Charlie" without tracking', () => + cleanReplace(paragraph, 'Alpha Bravo', 'Charlie', { 'word/endnotes.xml': ENDNOTES_XML })); + + await then('the response warns that endnote reference 1 was removed', () => { + expect(result.afterText).toBe('Charlie'); + expect(result.warnings).toEqual([ + 'The replaced range spanned an endnote reference (note id 1) not shown in the paragraph text; it was removed with the replaced text. Its note body stays in the notes part, no longer referenced.', + ]); + expect(result.savedXml).not.toContain('endnoteReference'); + }); + }); + + test('a range spanning both a field and a note reference reports each in document order', async ({ given, when, then }: AllureBddContext) => { + const paragraph = await given('a paragraph with an XE field and then a footnote reference inside the same range', () => + `${T('Alpha ')}${XE_FIELD}${T('Bravo')}${FOOTNOTE_REF}${T(' Charlie')}`); + + const result = await when('replace_text replaces the whole paragraph text', () => + cleanReplace(paragraph, 'Alpha Bravo Charlie', 'Delta', { 'word/footnotes.xml': FOOTNOTES_XML })); + + await then('two warnings name the field first and the note second', () => { + expect(result.afterText).toBe('Delta'); + expect(result.warnings).toHaveLength(2); + expect(result.warnings[0]).toContain('field with no result (instruction: XE "Alpha")'); + expect(result.warnings[1]).toContain('footnote reference (note id 1)'); + }); + }); + + test('control: an edit that removes none of these raises no new warning', async ({ given, when, then }: AllureBddContext) => { + const paragraph = await given('a paragraph with an XE field and a footnote reference after the edited word', () => + `${T('Alpha ')}${XE_FIELD}${T('Bravo')}${FOOTNOTE_REF}${T(' Charlie')}`); + + const beforeField = await when('replace_text edits only the text before the field', () => + cleanReplace(paragraph, 'Alpha', 'Alfa', { 'word/footnotes.xml': FOOTNOTES_XML })); + const betweenThem = await when('replace_text edits only the text between the field and the note reference', () => + cleanReplace(paragraph, 'Bravo', 'Beta', { 'word/footnotes.xml': FOOTNOTES_XML })); + const afterNote = await when('replace_text edits only the text after the note reference', () => + cleanReplace(paragraph, 'Charlie', 'Gamma', { 'word/footnotes.xml': FOOTNOTES_XML })); + + await then('no warning is raised and the field and reference are still in the saved paragraph', () => { + for (const result of [beforeField, betweenThem, afterNote]) { + expect(result.warnings).toEqual([]); + expect(result.savedXml).toContain('XE "Alpha"'); + expect(result.savedXml).toContain('footnoteReference'); + } + }); + }); + + test('control: a field whose cached result is in the paragraph text is not reported as hidden', async ({ given, when, then }: AllureBddContext) => { + const paragraph = await given('a paragraph "See ⟨REF Sec1⟩Section 1 above" whose field result the caller can see', () => + `${T('See ')}${REF_FIELD_WITH_RESULT}${T(' above')}`); + + const result = await when('replace_text edits the cached result "Section 1" to "Section 2"', () => + cleanReplace(paragraph, 'Section 1', 'Section 2')); + + await then('no hidden-construct warning is raised and the field is still there', () => { + expect(result.afterText).toBe('See Section 2 above'); + expect(result.savedXml).toContain('REF Sec1'); + expect(result.warnings.filter((w) => w.includes('not shown in the paragraph text'))).toEqual([]); + }); + }); + + test('tracked mode: the same range over an XE field says the field was deleted as a tracked change', async ({ given, when, then }: AllureBddContext) => { + const mgr = createTestSessionManager({ defaultAiAuthor: 'SafeDocX' }); + const session = await given('the XE paragraph opened with tracked emission', () => + openSession([], { mgr, xml: wrapDoc(`${T('Alpha ')}${XE_FIELD}${T('Bravo')}`) })); + + const replaced = await when('replace_text replaces "Alpha Bravo" with "Charlie"', () => + replaceText(mgr, { + file_path: session.inputPath, + target_paragraph_id: session.paraIds[1]!, + old_string: 'Alpha Bravo', + new_string: 'Charlie', + instruction: 'edit', + })); + + await then('the warning names the instruction and points at rejecting the deletion', () => { + assertSuccess(replaced, 'replace_text'); + const warnings = (replaced as { warnings?: string[] }).warnings ?? []; + expect(warnings).toEqual([ + 'The replaced range spanned a field with no result (instruction: XE "Alpha") not shown in the paragraph text; it was deleted with the replaced text as a tracked change; reject the deletion to restore.', + ]); + }); + }); +}); From 460373d09003b32adfc54e26411de38fb7523065 Mon Sep 17 00:00:00 2001 From: Steven Obiajulu Date: Mon, 28 Sep 2026 11:54:34 -0400 Subject: [PATCH 2/4] docs(docx-mcp): say hidden-construct labels follow field end markers, not document order Codex review of #1113: the collector emits a nested field before the field around it because each label is pushed when its end marker is reached. The comment claimed document order. Comment only; no behaviour change. Ref: #1097 --- packages/docx-mcp/src/tools/replace_text.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/docx-mcp/src/tools/replace_text.ts b/packages/docx-mcp/src/tools/replace_text.ts index 15658203..f17fcf58 100644 --- a/packages/docx-mcp/src/tools/replace_text.ts +++ b/packages/docx-mcp/src/tools/replace_text.ts @@ -70,8 +70,10 @@ function insideTrackedDeletion(el: Element, paragraph: Element): boolean { * without the caller having seen them (issue #1097): a field with no result * (an `XE` index entry, a `TC` entry, a `PAGE` never updated: complex fields * with no `separate` marker) and a footnote or endnote reference. Each is - * described once, in document order, so a before/after comparison names - * exactly what the edit removed. Fields with a cached result are left out: + * described once, in the order it is closed (a note reference where it sits, + * a field at its `end` marker, so a nested field precedes the field around + * it), and a before/after comparison names exactly what the edit removed. + * Fields with a cached result are left out: * their result is in the paragraph text the caller matched, so the caller saw * what the range covers. An empty `w:fldSimple` is a zero-length marker the * replace primitive keeps in place, so it needs no count. From 3adfd4be4af2ef5b1219ac53e3b0ad882ef2c039 Mon Sep 17 00:00:00 2001 From: Steven Obiajulu Date: Thu, 1 Oct 2026 15:38:38 -0400 Subject: [PATCH 3/4] docs(changelog): note no-op accept/reject and hidden-construct replace warnings --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e8f4a46..85aed989 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,20 @@ section break where the revised document removed a section break that had its own page settings. The document's final section settings are unchanged. (#981) +- `accept_ai_edits` / `reject_ai_edits` called with `revision_ids` that match + no revision no longer report `persistence_required: true` or a save + `next_step`, no longer echo the unknown ids in `selected_revision_ids` (now + `[]`), and no longer record a selective action on the session, so a following + `save` with `save_format: 'clean'` succeeds instead of failing with + `SELECTIVE_REVISIONS_WOULD_BE_DISCARDED`. A real selective accept/reject still + blocks a clean save that would discard the remaining AI revisions. (#1099) +- `replace_text` now warns when the replaced range removes a construct the + paragraph text does not show: a field with no result (for example an `XE` or + `TC` entry), named by its instruction, or a footnote/endnote reference, named + by its note id. One warning per construct removed, in the same style as the + existing symbol-character warning; in tracked mode the warning says it was + deleted as a tracked change. Fields with a cached result are not reported. + (#1097) ## 0.22.0 From b307f2fba58550d358128638147d8ccd95c9fad5 Mon Sep 17 00:00:00 2001 From: Steven Obiajulu Date: Thu, 1 Oct 2026 15:57:06 -0400 Subject: [PATCH 4/4] fix(docx-core): report only revision ids that exist as selected Selective accept/reject by revision_ids echoed every requested id, so a call mixing a real id with an unknown one reported and recorded the unknown id as selected. Report the requested ids that name a tracked change present in the swept stories; the filter applied to the document is unchanged. --- CHANGELOG.md | 6 ++-- .../src/primitives/accept_ai_edits.test.ts | 8 ++++++ .../src/primitives/accept_ai_edits.ts | 28 ++++++++++++++++--- packages/docx-core/src/primitives/document.ts | 18 ++++++++---- .../docx-mcp/src/tools/accept_ai_edits.ts | 4 +-- .../add_selective_ai_accept_reject.test.ts | 23 +++++++++++++++ .../docx-mcp/src/tools/reject_ai_edits.ts | 4 +-- 7 files changed, 75 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85aed989..6dd0f069 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,8 +22,10 @@ `next_step`, no longer echo the unknown ids in `selected_revision_ids` (now `[]`), and no longer record a selective action on the session, so a following `save` with `save_format: 'clean'` succeeds instead of failing with - `SELECTIVE_REVISIONS_WOULD_BE_DISCARDED`. A real selective accept/reject still - blocks a clean save that would discard the remaining AI revisions. (#1099) + `SELECTIVE_REVISIONS_WOULD_BE_DISCARDED`. When only some of the requested ids + exist, `selected_revision_ids` lists just those. A real selective + accept/reject still blocks a clean save that would discard the remaining AI + revisions. (#1099) - `replace_text` now warns when the replaced range removes a construct the paragraph text does not show: a field with no result (for example an `XE` or `TC` entry), named by its instruction, or a footnote/endnote reference, named diff --git a/packages/docx-core/src/primitives/accept_ai_edits.test.ts b/packages/docx-core/src/primitives/accept_ai_edits.test.ts index 1b6a553d..918c8166 100644 --- a/packages/docx-core/src/primitives/accept_ai_edits.test.ts +++ b/packages/docx-core/src/primitives/accept_ai_edits.test.ts @@ -45,6 +45,14 @@ describe('selective accept/reject by revision id/author (#123)', () => { expect([...ids].sort()).toEqual(['1', '2']); }); + it('reports only the selected ids that name a revision present in the story', () => { + const doc = body(`${aiIns(1, 'a')}${humanIns(2, 'b')}`); + const { selectedIds } = acceptAIEdits(doc, { revisionIds: [999, 1] }); + expect(selectedIds).toEqual(['1']); + const none = rejectAIEdits(body(`${aiIns(1, 'a')}`), { revisionIds: [999] }); + expect(none.selectedIds).toEqual([]); + }); + it('resolves an author to the ids of every revision it authored', () => { const doc = body(`${aiIns(1, 'a')}${humanIns(2, 'b')}${aiDel(3, 'c')}`); const ids = resolveSelectedIds(collectRevisionElements(doc), { author: AI }); diff --git a/packages/docx-core/src/primitives/accept_ai_edits.ts b/packages/docx-core/src/primitives/accept_ai_edits.ts index 72e9bce7..7de43481 100644 --- a/packages/docx-core/src/primitives/accept_ai_edits.ts +++ b/packages/docx-core/src/primitives/accept_ai_edits.ts @@ -170,6 +170,22 @@ export function resolveSelectedIds(revisionElements: Element[], selector: AiEdit throw new Error('AiEditSelector requires either revisionIds or author.'); } +/** + * The selected ids that name a tracked change actually present in + * `revisionElements`, in selection order. `resolveSelectedIds` keeps requested + * `revisionIds` verbatim (an unknown id simply filters nothing); this is what a + * caller should report as selected, so an id that matches no revision is not + * echoed back as if it had been acted on (#1099). + */ +export function matchedSelectedIds(revisionElements: Element[], selectedIds: Set): string[] { + const present = new Set(); + for (const el of revisionElements) { + const id = revisionElementId(el); + if (id != null) present.add(id); + } + return [...selectedIds].filter((id) => present.has(id)); +} + /** * Find ambiguous overlaps within a single story: a targeted content-wrapper * revision that structurally contains — or is contained by — a non-targeted @@ -270,11 +286,13 @@ export function selectedIdFilter(selectedIds: Set): RevisionFilter { export function acceptAIEdits(doc: Document, selector: AiEditSelector): SelectiveAcceptResult { const root = storyRoot(doc); if (!root) return { result: emptyAccept(), selectedIds: [], overlaps: [] }; - const selectedIds = resolveSelectedIds(collectRevisionElements(root), selector); + const revisionElements = collectRevisionElements(root); + const selectedIds = resolveSelectedIds(revisionElements, selector); const overlaps = selector.normalizeFirst ? [] : detectAmbiguousOverlaps(root, selectedIds); if (overlaps.length > 0) throw new AmbiguousRevisionOverlapError(overlaps); + const matched = matchedSelectedIds(revisionElements, selectedIds); const result = acceptChanges(doc, { filter: selectedIdFilter(selectedIds) }); - return { result, selectedIds: [...selectedIds], overlaps }; + return { result, selectedIds: matched, overlaps }; } /** @@ -284,11 +302,13 @@ export function acceptAIEdits(doc: Document, selector: AiEditSelector): Selectiv export function rejectAIEdits(doc: Document, selector: AiEditSelector): SelectiveRejectResult { const root = storyRoot(doc); if (!root) return { result: emptyReject(), selectedIds: [], overlaps: [] }; - const selectedIds = resolveSelectedIds(collectRevisionElements(root), selector); + const revisionElements = collectRevisionElements(root); + const selectedIds = resolveSelectedIds(revisionElements, selector); const overlaps = selector.normalizeFirst ? [] : detectAmbiguousOverlaps(root, selectedIds); if (overlaps.length > 0) throw new AmbiguousRevisionOverlapError(overlaps); + const matched = matchedSelectedIds(revisionElements, selectedIds); const result = rejectChanges(doc, { filter: selectedIdFilter(selectedIds) }); - return { result, selectedIds: [...selectedIds], overlaps }; + return { result, selectedIds: matched, overlaps }; } function emptyAccept(): AcceptChangesResult { diff --git a/packages/docx-core/src/primitives/document.ts b/packages/docx-core/src/primitives/document.ts index c900f612..4be101ee 100644 --- a/packages/docx-core/src/primitives/document.ts +++ b/packages/docx-core/src/primitives/document.ts @@ -83,6 +83,7 @@ import { rejectChanges as rejectChangesImpl, type RejectChangesResult } from './ import { collectRevisionElements, resolveSelectedIds, + matchedSelectedIds, detectAmbiguousOverlaps, selectedIdFilter, AmbiguousRevisionOverlapError, @@ -873,7 +874,11 @@ export class DocxDocument { */ private async prepareSelectiveStories( selector: AiEditSelector, - ): Promise<{ stories: Array<{ path: string | null; doc: Document }>; selectedIds: Set }> { + ): Promise<{ + stories: Array<{ path: string | null; doc: Document }>; + selectedIds: Set; + matchedIds: string[]; + }> { const stories: Array<{ path: string | null; doc: Document }> = [ { path: null, doc: this.documentXml }, ]; @@ -891,7 +896,8 @@ export class DocxDocument { ); if (overlaps.length > 0) throw new AmbiguousRevisionOverlapError(overlaps); } - return { stories, selectedIds }; + // Computed before the sweep removes the selected revisions. + return { stories, selectedIds, matchedIds: matchedSelectedIds(allRevisionElements, selectedIds) }; } /** @@ -901,7 +907,7 @@ export class DocxDocument { * `normalizeFirst` is set. */ async acceptAIEdits(selector: AiEditSelector): Promise<{ result: AcceptChangesResult; selectedIds: string[] }> { - const { stories, selectedIds } = await this.prepareSelectiveStories(selector); + const { stories, selectedIds, matchedIds } = await this.prepareSelectiveStories(selector); const filter = selectedIdFilter(selectedIds); const total = emptyAcceptChangesResult(); @@ -930,7 +936,7 @@ export class DocxDocument { this.dirty = true; this.documentViewCache = null; } - return { result: total, selectedIds: [...selectedIds] }; + return { result: total, selectedIds: matchedIds }; } /** @@ -939,7 +945,7 @@ export class DocxDocument { * Symmetric to {@link DocxDocument.acceptAIEdits}. */ async rejectAIEdits(selector: AiEditSelector): Promise<{ result: RejectChangesResult; selectedIds: string[] }> { - const { stories, selectedIds } = await this.prepareSelectiveStories(selector); + const { stories, selectedIds, matchedIds } = await this.prepareSelectiveStories(selector); const filter = selectedIdFilter(selectedIds); const total = emptyRejectChangesResult(); @@ -968,7 +974,7 @@ export class DocxDocument { this.dirty = true; this.documentViewCache = null; } - return { result: total, selectedIds: [...selectedIds] }; + return { result: total, selectedIds: matchedIds }; } removeJuniorBookmarks(): number { diff --git a/packages/docx-mcp/src/tools/accept_ai_edits.ts b/packages/docx-mcp/src/tools/accept_ai_edits.ts index 8775d12c..f6d864c7 100644 --- a/packages/docx-mcp/src/tools/accept_ai_edits.ts +++ b/packages/docx-mcp/src/tools/accept_ai_edits.ts @@ -41,8 +41,8 @@ export async function acceptAiEdits( // a call that changed nothing has nothing to persist: it records no // selective action (which would make a later clean save fail with // SELECTIVE_REVISIONS_WOULD_BE_DISCARDED) and echoes no ids as selected. - // docx-core echoes requested revision_ids whether or not they exist, so - // the result counts, not the echoed ids, are what prove a change (#1099). + // The result counts are what prove a change; selectedIds lists only the + // requested ids that named a revision present in the document (#1099). const changed = revisionResultChangedDocument(result); const effectiveIds = changed ? selectedIds : []; if (changed) { diff --git a/packages/docx-mcp/src/tools/add_selective_ai_accept_reject.test.ts b/packages/docx-mcp/src/tools/add_selective_ai_accept_reject.test.ts index a13d4352..90cb973e 100644 --- a/packages/docx-mcp/src/tools/add_selective_ai_accept_reject.test.ts +++ b/packages/docx-mcp/src/tools/add_selective_ai_accept_reject.test.ts @@ -458,6 +458,29 @@ describe('No-op accept/reject by unknown revision id has nothing to persist (#10 }); } + for (const tool of [ + { name: 'accept_ai_edits', call: acceptAiEdits, realId: '101' }, + { name: 'reject_ai_edits', call: rejectAiEdits, realId: '103' }, + ] as const) { + test(`${tool.name} with one real and one unknown revision id reports and records only the real id`, async ({ given, when, then }: AllureBddContext) => { + const opened = await given('a session with AI and reviewer revisions', () => + openSession([], { mgr: manager(), xml: documentXml(MIXED_AUTHOR_BODY) }), + ); + const session = await docxSession(opened.mgr, opened.filePath); + + const result = await when(`${tool.name} targets one existing and one nonexistent revision id`, () => + tool.call(opened.mgr, { file_path: opened.filePath, revision_ids: [tool.realId, 999] }), + ); + + await then('the unknown id is neither reported as selected nor recorded on the session', () => { + assertSuccess(result, tool.name); + expect(result.selected_revision_ids).toEqual([tool.realId]); + expect(result.persistence_required).toBe(true); + expect(session.selectiveRevisionAction).toMatchObject({ tool: tool.name, selectedRevisionIds: [tool.realId] }); + }); + }); + } + test('a real selective accept still records the action and blocks a clean save that would discard remaining AI revisions (control)', async ({ given, when, then }: AllureBddContext) => { const opened = await given('a session with AI and reviewer revisions', () => openSession([], { mgr: manager(), xml: documentXml(MIXED_AUTHOR_BODY) }), diff --git a/packages/docx-mcp/src/tools/reject_ai_edits.ts b/packages/docx-mcp/src/tools/reject_ai_edits.ts index 1514bbe2..3285443a 100644 --- a/packages/docx-mcp/src/tools/reject_ai_edits.ts +++ b/packages/docx-mcp/src/tools/reject_ai_edits.ts @@ -42,8 +42,8 @@ export async function rejectAiEdits( // a call that changed nothing has nothing to persist: it records no // selective action (which would make a later clean save fail with // SELECTIVE_REVISIONS_WOULD_BE_DISCARDED) and echoes no ids as selected. - // docx-core echoes requested revision_ids whether or not they exist, so - // the result counts, not the echoed ids, are what prove a change (#1099). + // The result counts are what prove a change; selectedIds lists only the + // requested ids that named a revision present in the document (#1099). const changed = revisionResultChangedDocument(result); const effectiveIds = changed ? selectedIds : []; if (changed) {