fix(docx-mcp): no-op accept/reject has nothing to persist; clean replace warns on hidden fields and note refs

CHANGELOG.md

@@ -17,6 +17,22 @@
   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`. 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
+  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
 

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(`<w:p>${aiIns(1, 'a')}${humanIns(2, 'b')}</w:p>`);
+      const { selectedIds } = acceptAIEdits(doc, { revisionIds: [999, 1] });
+      expect(selectedIds).toEqual(['1']);
+      const none = rejectAIEdits(body(`<w:p>${aiIns(1, 'a')}</w:p>`), { revisionIds: [999] });
+      expect(none.selectedIds).toEqual([]);
+    });
+
     it('resolves an author to the ids of every revision it authored', () => {
       const doc = body(`<w:p>${aiIns(1, 'a')}${humanIns(2, 'b')}${aiDel(3, 'c')}</w:p>`);
       const ids = resolveSelectedIds(collectRevisionElements(doc), { author: AI });

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>): string[] {
+  const present = new Set<string>();
+  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<string>): 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 {

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<string> }> {
+  ): Promise<{
+    stories: Array<{ path: string | null; doc: Document }>;
+    selectedIds: Set<string>;
+    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 {

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.
+    // 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) {
+      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));

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,137 @@ 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);
+      });
+    });
+  }
+
+  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) }),
+    );
+    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');
+    });
+  });
+});

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.
+    // 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) {
+      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));