Conversation
run_stdio() read sys.stdin and wrote sys.stdout with whatever encoding the locale assigned them. On Windows that is the ANSI codepage (cp1251 on a Russian host), while MCP stdio is UTF-8 by specification and every client sends UTF-8. The failure was silent rather than loud, which is what made it costly: non-ASCII arguments decoded into mojibake, so docx_search_text returned count: 0 for a Russian query with no error at all, and a write operation would have stored corrupted text under a successful-looking result. Reconfigure stdin and stdout to UTF-8 before the first read. PYTHONUTF8 and PYTHONIOENCODING do not help for the distributed PyInstaller-frozen binary, because the variables never reach the interpreter, so the fix has to live in the server itself. Tested on Windows 10 (cp1251 locale) against a 63-paragraph document: a Russian query returned count: 0 before and count: 1 after. The new regression script emulates a locale-codepage console with explicit cp1251 stream wrappers, so it fails before this change and passes after it on any platform, including a UTF-8 CI runner. Co-Authored-By: Claude Code <noreply@anthropic.com>
The isolation checks in docx_validate only run when touched scopes are known, and they were the caller's to supply. Two consequences: a hand-written half of the claim replaced the audit-derived one and turned a correct edit into protected_* errors, and supplying nothing when no audit exists silently skipped the deep checks while still reporting ok: true. - merge explicit touched_* with the audit instead of letting them replace it - refuse to validate when neither is available, unless the caller opts in - report isolation_checks performed/skipped and how the claim was assembled Also fixes two defects found while verifying the above: - _canonical_hash used inclusive c14n, so a part that merely redeclares a namespace changed the hash of every element beneath it. The .NET backend hoists drawing namespaces, so validating a .NET-edited document with the Python backend reported every paragraph as protected_paragraph_changed. Exclusive c14n renders only the namespaces an element actually uses. - normalize_patchset rejected a patchset sent as a JSON string, which is what clients that do not expand the "$ref" in the tool schema send. Co-Authored-By: Claude Code <noreply@anthropic.com>
The test drives the server-level validation path directly, so it needs no .NET backend and no fixture document. It covers refusing an unavailable claim, reporting isolation_checks=skipped when the caller opts out, merging an explicit claim with the audit, diagnosing a single-axis claim, hashing equivalent markup equally, and accepting a JSON-string patchset. It fails on the previous commit at the first assertion. Co-Authored-By: Claude Code <noreply@anthropic.com>
Two defects that an agent meets before any of the transport bugs, since they are the documented anchor and the documented pre-flight step. paraId could not be used as a write target on the .NET backend: it failed with target_resolution_failed: Target paragraph not found, while paragraph_index resolved to the very same id and the engine reported that id back in touched.para_ids. The Python backend resolved both. paraId is declared throughout the PatchSet schema and SKILL.md ranks it above paragraph_index, so the documented first choice was the one that did not work. The .NET engine ships prebuilt and cannot be rebuilt here, so the facade translates the anchor (ooxml.resolve_paraid_targets, called from the dotnet_* entry points) instead of the caller having to know which backend is in play. paraId stays the more precise anchor - an index shifts when content is inserted earlier in the body - so it is resolved against the source document rather than discarded, it wins when an operation carries both axes as it already did in Python, and paragraphs inside tables resolve too. An id that is not in the document is left untouched so the engine, not the facade, reports it. assess_patchset resolved every target and then checked only that a precondition was present (is None), never that it held, so it answered ok: true, risks: [] on both backends for an expected_old_sha256 of all zeroes - and for a table's xml_sha256, which docx_list_tables offers right beside the text_sha256 a precondition actually needs - while the write path refused that same edit. Assessment blessing an edit the writer rejects is the worst answer this tool can give. It now compares through _assert_expected_text, the very call the write path makes, so the two cannot drift apart again; the .NET result is corrected in the facade, since that backend cannot be rebuilt; and the schema now names the hash a precondition needs. Covered by scripts/run_paraid_target_regression.py (15 assertions) and scripts/run_precondition_assessment_regression.py (10 cases plus the .NET merge, driven through a stand-in backend so it needs no .NET), both added to CONTRIBUTING.md and to the Python checks in ci.yml. Both fail on the previous commit. Co-Authored-By: Claude Code <noreply@anthropic.com>
Unit-level coverage exists for both transport bugs (run_stdio_encoding_ regression.py mocks the server; run_validation_isolation_regression.py calls normalize_patchset directly), but nothing exercised a real server process writing a real document. This spawns word_ai_mcp.server over stdio and verifies: a string patchset is accepted end-to-end, Cyrillic and non-cp1251 characters (Greek Sigma, arrows, checkmark) survive the write byte-exact, a Cyrillic search query matches the written text, validate reports only word/document.xml changed, and the source sha256 is stable. Defaults to WORD_AI_ENGINE=python so the test does not require a .NET SDK; an explicit engine in the caller environment is respected. Co-Authored-By: Claude Code <noreply@anthropic.com>
Two smaller inconsistencies of the same shape as the assessment bug: the tool knew the answer and did not put it where the caller reads. docx_dry_run_patchset carried its verdict only inside safety_assessment.ok and validation.ok, while docx_assess_patchset, docx_validate and docx_compare_structure each answer with a top-level ok. A caller checking result["ok"], as it does for the others, met a KeyError - and one reading the absence as a failure calls a successful dry run bad. The verdict is now the conjunction of the two sub-verdicts, in both backends, since the .NET result needs the same treatment. An absent sub-verdict counts as not-ok: an unproven pass is the one answer this tool must never give. A tool reads its required arguments as args["docx_path"], so a caller who guesses the parameter name - "path", say - was answered with KeyError: 'docx_path' and a Python traceback, which says nothing about what the tool accepts. The dispatcher now names the missing argument together with the tool's own required list. A KeyError that is not a declared argument - a tool's internal data lookup - keeps its original traceback, since that is not the caller's mistake to fix. Covered by scripts/run_tool_verdict_regression.py, added to CONTRIBUTING.md and to the Python checks in ci.yml. The argument hint is asserted through the request handler rather than the helper, so a helper that stops being wired in is caught; removing the dry-run wrapper fails the same script with a KeyError on "ok". Co-Authored-By: Claude Code <noreply@anthropic.com>
The script landed in f52ac43 but was left out of the check lists. It runs the python engine by default so it needs no .NET SDK, matching the other transport-level checks; verified locally with PYTHONPATH=. before wiring (5 cases ok, compileall clean). Co-Authored-By: Claude Code <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Seven defects, all found while editing a real 4.6 MB Russian research document (1181 paragraphs, 26 tables, 126 fields, scientific symbols). Five of them make the tool report a confident, wrong answer — or report no answer — rather than fail, which is the worst shape this class of bug takes for a tool whose whole promise is verified surgical editing.
Three are on the Windows/cp1251 path. The other four were found while verifying those, and are the ones an agent meets first, since they are the documented anchor, the documented pre-flight step, and the verdict fields it reads afterwards.
1. stdio used the process locale instead of UTF-8
run_stdio()readsys.stdinand wrotesys.stdoutwith whatever encoding the process locale assigned them. Python defaults those streams to the locale encoding: on Windows that is the ANSI codepage (cp1251 on a Russian host). MCP stdio is UTF-8 by specification, and Claude Code, OpenCode and Codex all send UTF-8 — so every non-ASCII argument was decoded as cp1251.The failure was silent, not loud:
docx_search_textwith a Russian query returnedcount: 0and no error at all, so it looked like a missing-text problem in the document rather than a transport problem.₃ ² → − ≥ Δ; 221 occurrences) died withUnicodeEncodeErrorwhile encoding the reply, so the document could not be read at all.This reconfigures both streams to UTF-8 before the first read.
2.
docx_validatecould report an unverified passThe isolation checks (
protected_*) only run when touched scopes are known, and they were entirely the caller's to supply:.audit.json, skipped the deep checks outright —metrics.protected_object_checkssaid"skipped"while the report still returnedok: true.touched_*replaced the audit-derived set rather than merging with it. The checks use two axes — paragraphs are matched byw14:paraId, body blocks by paragraph index — so a caller who passed one axis made the other check compare against an empty set and reportedprotected_paragraph_changedorprotected_body_block_changedfor an edit that changed nothing else.Measured on a one-paragraph edit, correct values
paraId=1810201C,index=236:touched_para_idsonlyprotected_body_block_changedtouched_paragraph_indicesonlyprotected_paragraph_changedok: true, nothing checkedallow_skipped_isolation_checks: trueisolation_checks: "skipped"+ warningExplicit scopes now merge into the audit instead of replacing it, validating with neither an audit nor a claim is refused unless the caller explicitly opts into an inconclusive result, and every report carries
isolation_checks: performed|skippedplustouched_scopes.3. Two defects found while verifying the above
_canonical_hashused inclusive c14n. Inclusive canonicalisation renders every in-scope namespace declaration, so a part that merely redeclares a namespace changes the hash of every element beneath it. The .NET backend hoists drawing namespaces (xmlns:a,xmlns:a14,xmlns:pic) into ancestor scope, so validating a .NET-edited document with the Python backend reported every paragraph and table as changed:protected_paragraph_changed+protected_table_changed+protected_body_block_changedon a correct single-paragraph edit. Same paragraph, inclusive c14n: 3215 bytes vs 3016; exclusive c14n: 675 vs 675, byte-identical. Exclusive c14n renders only the namespaces an element actually uses.normalize_patchsetrejected a patchset sent as a JSON string, which is what a client sends when it does not expand the"$ref"in the tool schema. Everyassess/dry_run/applycall from such a client failed withpatchset must be an objecteven for a valid payload. It now parses the string first, and still refuses junk with a clear message.4.
paraIdwas a documented anchor that could not be written toparaIdis declared throughoutschemas/patchset.schema.json, andSKILL.mdranks it aboveparagraph_indexin anchor priority — so the documented first choice was the one that did not work. On the .NET backend, addressing a paragraph by itsw14:paraIdfailed withtarget_resolution_failed: Target paragraph not found, whileparagraph_indexresolved to that very same id and the engine reported the id back intouched.para_ids. The Python backend resolved both.The .NET engine ships prebuilt and cannot be rebuilt here, so the facade translates the anchor instead of the caller having to know which backend is in play:
paragraph_indexparaIdtarget_resolution_failedparaIdstays the more precise anchor — an index shifts when content is inserted earlier in the body — so it is resolved against the source document rather than discarded, it wins when an operation carries both axes (as it already did in Python), and paragraphs inside tables resolve too. An id that is not in the document is left untouched so the engine, not the facade, reports the missing target.5.
assesscould bless a PatchSet the write path then refusedAssessment resolved every target and then checked only that a precondition was present (
is None), never that it held. It answeredok: true, risks: []— on both backends — for anexpected_old_sha256of all zeroes, and for a table'sxml_sha256, whileapplyrefused that same edit. Verified on a one-table edit of the 4.6 MB document:expected_old_sha256assessassessapplytext_sha256ok: trueok: truexml_sha256ok: trueprecondition_mismatch"0" * 64ok: trueprecondition_mismatchThe table case is a genuine trap rather than carelessness on the caller's part:
docx_list_tablesofferstext_sha256andxml_sha256side by side,expected_old_sha256hashes the target's text, and nothing said which was which — the schema declared the field as a bare"type": "string". Assessment now compares through_assert_expected_text, the very call the write path makes, so the two cannot drift apart again; the .NET result is corrected in the facade, since that backend cannot be rebuilt; and the schema now names the hash a precondition needs.6. A dry run reported no verdict at the top level
docx_assess_patchset,docx_validateanddocx_compare_structureeach answer with a top-levelok.docx_dry_run_patchsetdid not — its verdict lived only insidesafety_assessment.okandvalidation.ok, on both backends. A client checkingresult["ok"], exactly as it does for the three siblings, met aKeyError; one that reads the missing key as falsy calls a successful dry run a failure. The verdict is now the conjunction of the two sub-verdicts, and an absent sub-verdict counts as not-ok, since an unproven pass is the one answer this tool must never give.7. A missing argument was answered with a bare
KeyErrorTools read their required arguments as
args["docx_path"], so a caller who guesses the parameter name —path, say — was answered withKeyError: 'docx_path'and a Python traceback, which says nothing about what the tool accepts. The dispatcher now names the missing argument together with the tool's own required list:A
KeyErrorthat is not a declared argument — a tool's own internal data lookup — keeps its original traceback, since that is not the caller's mistake to fix.Test
Four scripts, added to
CONTRIBUTING.mdand to the Python checks inci.yml:scripts/run_validation_isolation_regression.py— drives the server-level validation path directly, so it needs no .NET backend and no fixture document. On the previous commit it fails at the first assertion:AssertionError: validation without an isolation claim should have been refused.scripts/run_paraid_target_regression.py— 15 assertions pinning the translation: the id resolves to the right index, a stale index yields to it, table paragraphs are covered, an unknown id is left for the engine, and a patchset that needed no translation is returned unchanged.scripts/run_precondition_assessment_regression.py— 9 cases over paragraphs, table rows, table cells and content-control-free anchors, assertingassessandapplyagree in both directions, sinceapply_patchsetrefuses wheneverassess_patchsetis not ok. It also drives the .NET merge through a stand-in backend, so it stays free of .NET and of the prebuilt engine.scripts/run_tool_verdict_regression.py— the dry-run verdict is the conjunction of both sub-verdicts and an absent sub-verdict is not a pass; a real dry run reports a top-levelok; and a missing argument is asserted throughhandle, not through the helper, so the test notices the helper going unwired.All four fail on the pre-fix commit. The stdio test substitutes loopback streams wrapped in explicit cp1251 rather than relying on the host locale, so it is deterministic on any platform — including a UTF-8 CI runner, where a locale-dependent test would pass even without the fix.
A fifth script on this branch,
scripts/run_e2e_stdio_utf8_regression.py, was written by a second contributor working alongside me. It passes; it is not wired intoci.ymlorCONTRIBUTING.md, which is theirs to decide.Checks
compileall,run_smoke_test,run_structure_regression,run_outline_regression,run_stdio_encoding_regression,run_validation_isolation_regression,run_paraid_target_regression,run_precondition_assessment_regression,run_tool_verdict_regression,run_word_session_smokeandvalidate_word_ai_skillall pass, and the tool surface is still 63.run_patchset_alias_regressionandrun_engine_selection_regressionfail on this machine for an unrelated reason — no .NET SDK installed (global.jsonSDK resolution failure); I confirmed the same failure on a cleanmaincheckout before opening this.Note on verification
The .NET backend re-serialises the whole
word/document.xmlpart, even for a one-paragraph edit: on the test document it grew from 982 912 to 994 014 bytes and all 1160 paragraph fragments differ — attribute order (w:rsid*),<w:spacing ... />spacing, and hoisted namespace declarations. Nothing semantic changes, and the package-level guarantee holds (every other part is byte-identical, which is what preserves images, styles, headers and numbering), but a byte-level diff insidedocument.xmlis not local. It may be worth documenting, since "only your paragraph changed" cannot be shown that way.Scope
Two things are deliberately not in this PR:
assesscouplesallow_paragraph_count_changeandallow_table_dimension_change: a table row insertion needs both, so a caller who set only the table flag is refused for the paragraph count. I left it alone. Splitting the two is a guard-rail semantics question — the coupling protects against an edit that changes a dimension nobody declared — and loosening it to save callers one flag is the wrong trade for a tool whose value is the refusal. It is worth a maintainer's judgement, not a silent widening here.word-ai httpwas reported to accept TCP connections and then never answer (GET /,/health,/mcpall hang to timeout, log file empty). I could not reproduce it —GET /200,GET /health200,POST /mcp200 with all 63 tools,GET /mcp404 — on either the prebuilt engine or a source checkout. Two things that look like the report are not it: an empty log file is stdout block buffering when the stream is redirected, and two processes apparently bound to one port is WindowsSO_REUSEADDR. Happy to take it further with the reporter'sword-ai --version, host and exact command.Happy to file any of these separately or fold them in on request.
🤖 Generated with Claude Code