[stevenobiajulu]

Why

#1099. accept_ai_edits / reject_ai_edits with revision_ids that match no revision reported persistence_required: true with a save instruction, echoed the unknown ids as selected_revision_ids, and recorded a selective action on the session, so a later save(save_format: 'clean') failed with SELECTIVE_REVISIONS_WOULD_BE_DISCARDED although nothing had changed. #1089 stopped such calls from bumping the edit counters but left the action record and the response fields. docx-core's resolveSelectedIds 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 (revisionResultChangedDocument, 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.

#1097. 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 the 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. An empty w:fldSimple is not counted because the clean replace primitive keeps it in place (covered by a control test).

The counting stays in the MCP layer; packages/docx-core is untouched (#1094/#1096 are changing its replace path in parallel). server.ts is untouched (#1090). No tool schema or description changed.

Tests

packages/docx-mcp/src/tools/add_selective_ai_accept_reject.test.ts — new describe block No-op accept/reject by unknown revision id has nothing to persist (#1099):

• both tools: unknown ids → persistence_required: false, no next_step, selected_revision_ids: [], session.selectiveRevisionAction null (AC 1)
• both tools: a clean save after the no-op call succeeds and its document.xml is byte-identical to a control session's clean save with no call (AC 2)
• accept and reject controls: a real selective call by id still records the action and the clean save is refused with SELECTIVE_REVISIONS_WOULD_BE_DISCARDED (AC 3)
• the docx-mcp: accept_changes and reject_ai_edits mark the session edited even when nothing changed #1084 test's stale comment about echoed ids is updated to assert [].

packages/docx-mcp/src/tools/replace_text_hidden_construct_warning.regression.test.ts (new):

• clean replace over an XE "Alpha" field → warning names the instruction; saved paragraph has no field (AC 1)
• clean replace over a footnote reference, and over an endnote reference → warning names the note id (AC 2)
• field and note in one range → two warnings in document order
• controls: edits before the field, between field and note, and after the note → no warning and both survive; an edit inside a cached REF result → no warning; an empty w:fldSimple inside the range survives with no warning (AC 3)
• tracked mode over the XE field → the warning says deleted as a tracked change.

Repro scripts from the follow-ups run (noop-selective.test.ts, replace-text-leads.test.ts) confirm the fixed behaviour on this branch.

Closes #1099
Closes #1097

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
site	Ready	Preview	Oct 1, 2026 8:06pm UTC

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-28T15:55:42.093256Z	0bc251d	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[usejunior-llm-gate]

LLM gate (advisory)

1 finding to review (advisory, does not block) - 7 pass, 1 warn, 0 error, 8 skipped, 16 total

Findings (1)

Rule	Verdict	Detail
Re-derived facts vs canonical sources	WARN	In packages/docx-mcp/src/tools/replace_text.ts:73, collectLiveHiddenConstructs re-implements a manual DOM walk for fldChar begin/separate/end state and footnote/endnote references, instead of consuming canonical sources like buildParagraphIndex (packages/docx-core/src/primitives/paragraph-index.ts:73) or getFootnoteMarkersForParagraph (packages/docx-core/src/primitives/document_view.ts:296). No explicit justification or agreement tests are provided.

All 16 rules (8 evaluated, 8 skipped)

Rule	Verdict	Detail
read_file response metadata parity	SKIPPED	paths not touched by this PR
Live DOM namespace-safe OOXML writes	SKIPPED	paths not touched by this PR
Complex-field revisions preserve complete accept/reject state machines	PASS	The PR does not touch field atomization, validateFieldStructure, w:delInstrText, or collapsed-field comparison logic, and its references to field characters in packages/docx-mcp/src/tools/replace_text.ts:80 are solely for diagnostic warnings on hidden constructs.
Field validation per story, not global	SKIPPED	paths not touched by this PR
Revision IDs seeded from all revision-bearing side parts	SKIPPED	paths not touched by this PR
Accept/reject sweep side parts and caches	PASS	All revision-bearing stories are swept by accept/reject using enumerateSelectedRevisionStoryPartPaths (revision-parts.ts:45). Updates are written back only on change while refreshing cached XML (document.ts:925), and pruneOrphanedFootnotes (document.ts:263) cleans orphaned footnotes while preserving reserved separators.
DocumentViewNode.heading stays canonical	SKIPPED	paths not touched by this PR
AI-author parity across entry points	SKIPPED	paths not touched by this PR
Property-change wrapper discipline	SKIPPED	paths not touched by this PR
SUPPORT.md Table A drift vs. implementation	PASS	The PR does not modify OOXML revision emission behavior or touch packages/docx-core/SUPPORT.md; it only updates the selective accept/reject tracked-change consumer filtering inside packages/docx-core/src/primitives/accept_ai_edits.ts and packages/docx-core/src/primitives/document.ts.
Table A / Table B boundary on side-part revisions	SKIPPED	paths not touched by this PR
Canonical-emission surface completeness	PASS	The PR modifies selective accept/reject logic in packages/docx-core/src/primitives/accept_ai_edits.ts and adds warnings for hidden construct removals in packages/docx-mcp/src/tools/replace_text.ts, but it does not add or change any tracked-edit emission surface (Table A).
Unit-test quality (avoid tautological / change-detector tests)	PASS	The PR adds high-quality unit and regression tests for #1099 and #1097, with independent assertions checking exact output text, warnings, and document XML structures, successfully avoiding tautological or change-detector patterns.
Re-derived facts vs canonical sources	WARN	In packages/docx-mcp/src/tools/replace_text.ts:73, collectLiveHiddenConstructs re-implements a manual DOM walk for fldChar begin/separate/end state and footnote/endnote references, instead of consuming canonical sources like buildParagraphIndex (packages/docx-core/src/primitives/paragraph-index.ts:73) or getFootnoteMarkersForParagraph (packages/docx-core/src/primitives/document_view.ts:296). No explicit justification or agreement tests are provided.
.openspec tag ↔ test-assertion drift	PASS	The PR does not add, move, or change any .openspec tags on any test, so the precondition for this checklist question is not met.
Library stays general (no downstream-domain leakage)	PASS	The PR adds general OOXML/LLM-driven helpers like matchedSelectedIds and collectLiveHiddenConstructs to handle selective track changes and hidden document features, with no leakage of downstream consumer or agreement domain concepts.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0bc251dcb0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Detect empty separated fields as hidden

A separate marker does not guarantee that the field has a visible cached result. For a valid begin → instrText → separate → end field with an empty result, the paragraph text still omits the field and the core replacement path treats its markers as deletable zero-width content, but this condition excludes it from the before/after comparison. Replacing text across such a field therefore silently removes it without the warning this change is intended to provide; track whether the result region contains visible content rather than only whether separate was encountered.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Move the complete field sequences into shared fixtures

REF_FIELD_WITH_RESULT reimplements a complete complex-field sequence even though ooxml-fixtures.ts already provides completeField and the underlying field primitives; the new XE constant similarly duplicates those primitives. Keeping these definitions inline allows the exact cross-file fixture drift the repository rule is designed to prevent, so add/reuse the shared helpers instead.

AGENTS.md reference: AGENTS.md:L61-L67

Useful? React with 👍 / 👎.

[stevenobiajulu]

Codex peer review (gpt-6-sol) — verdict and adjudication

Verdict: APPROVE, no blocking finding. Reviewer ran the five cited vitest files (39 passed), seven temporary probes of its own (nested and duplicate XE fields, markup path, normalize_first, w:rPrChange-only selector, mixed known/unknown ids, note body retention), and npm run check:tool-docs (exit 0); probes were removed and git status was clean afterwards. Full pre-submit was run by me, not the reviewer (exit 0 on 0bc251d; rerun on 4caa530 below).

#	Finding	Adjudication
1	Nonblocking: with mixed ids ([101, 999]) where 101 changes the document, selected_revision_ids still echoes 999.	Accepted as a known limit, not changed. docx-core's resolveSelectedIds echoes requested ids without checking existence, and this PR must not touch docx-core (#1094/#1096 are changing its replace path in parallel). The issue lists filtering as "ideally". The clean-save guard fires correctly in the mixed case (reviewer confirmed). Filtering in docx-core is a one-line follow-up once that stream lands.
2	Nonblocking: the collector's comment says labels are "in document order", but a nested field is labelled at its end marker, so the inner field precedes the outer one.	Fixed in 4caa530 (comment only; behaviour unchanged, both fields were named correctly in the reviewer's probe).

[codecov]

Codecov Report

❌ Patch coverage is 92.64706% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/docx-mcp/src/tools/replace_text.ts	90.00%	0 Missing and 4 partials ⚠️
...ckages/docx-core/src/primitives/accept_ai_edits.ts	92.30%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[stevenobiajulu]

CI status (needs a human decision). workspace-test (20) on 4caa530 failed only at the coverage ratchet step: @usejunior/docx-mcp line baseline is stale: current 88.70% exceeds baseline 87.69% by 1.01 points (all 1061 docx-mcp tests and every other suite passed; workspace-test (22) does not run the ratchet). The new #1099/#1097 tests pushed the package past the 1-point positive-drift limit, so the ratchet asks for a rebaseline (npm run coverage:packages:rebaseline).

I prepared the baseline bump (docx-mcp lines 88.7, branches 78.82, functions 94, statements 86.49 — the values job 109022085343 measured; docx-core and docx-compare unchanged) but did not commit or push it: the agent permission check classified verifying that change as a CI bypass, so it is left uncommitted in the worktree for a human to apply. Correction: an earlier version of this comment claimed the bump was committed as 4caa530; it was not, 4caa530 is the comment-only fix from the Codex review.

[stevenobiajulu]

Codex peer review (gpt-6-sol): adjudication

Rebased onto origin/main (which now includes #1090). Added a CHANGELOG entry under ## Unreleased covering #1099 and #1097.

Round 1 (at cc90757e): CHANGES_REQUIRED, one Medium finding.

• Finding: a call that mixed one real and one unknown id (revision_ids: [101, 999]) still reported selected_revision_ids: ["101","999"] and recorded both on the session's selective action. docx-core's resolveSelectedIds keeps requested ids verbatim. Accepted. I verified it with sed -n 155,170p packages/docx-core/src/primitives/accept_ai_edits.ts, and the issue asks that unknown ids not be echoed. Fixed in baa5337b. A new matchedSelectedIds() in docx-core returns the requested ids that name a tracked change present in the swept stories, computed before the sweep. DocxDocument.acceptAIEdits / rejectAIEdits and the single-story primitives return that list as selectedIds. The filter applied to the document still uses the full requested set, so which elements get swept is unchanged; only the reported and recorded list changes. New tests: a partial-match case for each MCP tool, and a docx-core primitive case. The CHANGELOG notes the partial-match behavior.
• Cross-paragraph fields (raised as unverified, no finding): the reviewer's probe fixture never reached the warning path. I reran it with a well-formed fixture: a field whose begin/instr is in one paragraph and its end in the next, then a clean replace in each paragraph. Both succeed with no warning and no error. The scan is paragraph-local and emits a label only for an end matched by a begin in the same paragraph. A field that crosses paragraphs can therefore be missed, but it cannot produce a false warning. Behavior there is unchanged from main.
• Also verified by the reviewer: xmldom getAttributeNS returns null when the attribute is absent, so the ?? fallback works.

Round 2 (confirmation, at baa5337b): APPROVE. The reviewer reran the partial-match probe against the rebuilt dist (selectedIds: ["101"] for both accept and reject) and probed move ids: wrapper ids are reported and resolved; range-marker ids report [] with no counted change. It found no caller that depends on unknown ids being echoed.

Local verification at baa5337b: npm run build, lint:workspaces, check:cycles, allure label/quality/filename checks, check:tool-docs, check:spec-coverage, docx-mcp test:run (118 files, 1084 tests), test:coverage:packages (docx-core 1571 pass) and coverage:packages:check all exit 0.

A pre-merge stdio smoke on a real Bonterms Mutual NDA passed. The post-merge smoke will follow.

[stevenobiajulu]

Rebased onto origin/main after #981 and #966 landed; the only conflict was CHANGELOG ## Unreleased, and both entries are kept. The code is unchanged from the approved baa5337b. Re-verified at b307f2fba58550d358128638147d8ccd95c9fad5: npm ci, build, lint:workspaces, check:cycles, docx-mcp test:run (1084 tests), and test:coverage:packages + coverage:packages:check all exit 0. Automerge is pinned to b307f2fba58550d358128638147d8ccd95c9fad5.

[stevenobiajulu]

Post-merge smoke: PASS

I built origin/main at the squash commit f545bdf5 (npm ci && npm run build), started the built stdio server (packages/safe-docx/bin/safe-docx.js), and drove it with a real MCP SDK client. The input was a copy of the Bonterms Mutual NDA template (open-agreements/templates/bonterms-cc0-1.0/bonterms-mutual-nda/template.docx).

• docx-mcp: accept_ai_edits/reject_ai_edits with ids that match nothing report persistence_required and then block a clean save #1099, no-op by unknown id: I made one tracked edit on the NDA, then called accept_ai_edits and, in a separate session, reject_ai_edits with revision_ids: ['987654321']. Each returned persistence_required: false, selected_revision_ids: [] and no next_step. A following save(save_format: 'clean') succeeded, and the output re-reads with the edit applied.
• docx-mcp: accept_ai_edits/reject_ai_edits with ids that match nothing report persistence_required and then block a clean save #1099 control: after a real selective accept_ai_edits by id, the response reports persistence_required: true with the id, and the clean save is still refused with SELECTIVE_REVISIONS_WOULD_BE_DISCARDED.
• replace_text: a clean edit removes a result-less field or a footnote reference without any warning #1097: the NDA has no fields or note references of its own, so I added a footnote reference to the cover-page paragraph with add_footnote. An untracked replace_text whose range spans the reference returns exactly one warning, naming footnote note id 1. A control replace elsewhere in the same paragraph has no warning, and the clean save succeeds. The XE/TC field case is covered by replace_text_hidden_construct_warning.regression.test.ts.

Raw output

=== accept_ai_edits with an unknown revision id
ok  : tracked replace_text on the NDA succeeds (warnings: [])
AI revisions present: total=1
{"success":true,"persistence_required":false,"selected_revision_ids":[]}
ok  : no-op call succeeds
ok  : persistence_required is false
ok  : no next_step
ok  : selected_revision_ids is []
{"success":true}
ok  : clean save after the no-op succeeds (24394 bytes)
ok  : clean output re-reads with the edit applied

=== reject_ai_edits with an unknown revision id
ok  : tracked replace_text on the NDA succeeds (warnings: [])
AI revisions present: total=1
{"success":true,"persistence_required":false,"selected_revision_ids":[]}
ok  : no-op call succeeds
ok  : persistence_required is false
ok  : no next_step
ok  : selected_revision_ids is []
{"success":true}
ok  : clean save after the no-op succeeds (24394 bytes)
ok  : clean output re-reads with the edit applied

=== control: real selective accept_ai_edits by id
first revision id: 2
{"success":true,"persistence_required":true,"next_step":"Call save with save_format='tracked' or 'both' to persist this session-scoped mutation.","selected_revision_ids":["2"]}
ok  : real selective accept reports persistence_required + selected ids
{"success":false,"error":{"code":"SELECTIVE_REVISIONS_WOULD_BE_DISCARDED","message":"Clean output would auto-accept 3 remaining revision(s) by SafeDocX after a selective revision operation.","hint":"Use save_format='tracked', finish accepting/rejecting the remaining revisions, or explicitly set allow_discard_preserved_revisions=true."}}
ok  : clean save is still refused after a real selective accept

=== clean replace_text spanning a footnote reference (SAFE_DOCX_AI_AUTHOR="")
{"success":true,"note_id":1}
ok  : add_footnote on the NDA cover page succeeds
{"success":true,"warnings":["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."]}
ok  : clean replace over the footnote reference warns once, naming the footnote reference
ok  : control replace elsewhere in the paragraph has no hidden-construct warning ([])
ok  : clean save succeeds

tmp=/private/var/folders/0q/zgzp789j36b9gscddw1lyxp80000gn/T/smoke1113-EHZdKi
SMOKE PASS