Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an opt-in Changes
Sequence Diagram(s)sequenceDiagram
actor Client as Claude Code
participant Server as "MCP Server\n(docx_editor_mcp.server)"
participant Cache as "DocumentCache\n(docx_editor_mcp.cache)"
participant Disk as Filesystem
Note over Client,Disk: Open document & caching
Client->>Server: open_document(path, author?)
Server->>Cache: get(normalize(path))
alt cached
Cache-->>Server: CachedDocument
else not cached
Server->>Disk: read file (open)
Disk-->>Server: document + mtime
Server->>Cache: put(CachedDocument)
end
Server-->>Client: success / hint
Note over Client,Cache: Edit and mark dirty
Client->>Server: replace_text(path, old, new)
Server->>Cache: get(path)
Cache-->>Server: CachedDocument
Server->>Server: apply edit (document API)
Server->>Cache: mark_dirty()
Server-->>Client: success (change_id)
Note over Client,Disk: Save with external-change check
Client->>Server: save_document(path)
Server->>Cache: get(path)
Cache-->>Server: CachedDocument
Server->>Disk: stat path -> mtime
Disk-->>Server: current mtime
alt external change detected
Server-->>Client: error (external)
else
Server->>Disk: write file (save)
Disk-->>Server: ok
Server->>Cache: update_mtime(), clear_dirty()
Server-->>Client: success
end
Note over Server,Cache: LRU eviction on capacity
Server->>Cache: put(new_doc) when full
Cache->>Cache: evict_lru()
alt evicted dirty
Cache->>Disk: save before eviction (best-effort)
Disk-->>Cache: ok / error
end
Cache-->>Server: eviction_done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@docx_editor_mcp/cache.py`:
- Around line 94-108: The put method normalizes cached_doc.path for the cache
key but stores the original CachedDocument object unchanged, causing path
inconsistency; update the CachedDocument's path to the normalized value before
storing (e.g., inside put: compute normalized = normalize_path(cached_doc.path)
and set cached_doc.path = normalized) or ensure CachedDocument constructor
always normalizes paths so that the object and cache key match; make this change
in the put method (and/or CachedDocument creation) so functions like
has_external_changes that rely on cached_doc.path see the normalized path.
- Around line 126-140: The eviction currently calls lru_doc.document.save() with
no error handling which can propagate exceptions and corrupt cache state; wrap
the save call in a try/except, catch Exception as e, log the failure (including
lru_path and the exception) using the same logger pattern as server.shutdown,
and if the save fails do NOT delete the entry from self._cache (leave it dirty
so the server can retry later); ensure the code references _evict_lru,
lru_doc.document.save(), and self._cache to locate and update the logic.
In `@docx_editor_mcp/server.py`:
- Around line 28-30: In the except block that currently calls
logger.error(f"Failed to save {cached_doc.path}: {e}"), replace that call with
logger.exception("Failed to save %s", cached_doc.path) (or logger.error("Failed
to save %s", cached_doc.path, exc_info=True)) so the traceback is recorded and
logging uses %-style formatting; this change should be made in the exception
handler around the save logic that references cached_doc.path.
- Around line 23-34: The shutdown method currently iterates self.cache.all()
twice (first to save dirty documents, then to remove entries); consolidate into
a single loop in shutdown that for each cached_doc checks cached_doc.dirty and
calls cached_doc.document.save() inside try/except (logging success or error
using cached_doc.path), then calls self.cache.remove(cached_doc.path)
unconditionally (or in finally) to ensure the entry is cleared; update
references to cached_doc, cached_doc.document.save(), self.cache.remove(...) and
logger to perform both actions in one pass.
In `@docx_editor_mcp/tools.py`:
- Around line 141-148: The cleanup currently swallows exceptions from
cached.document.close() (seen in the block after the comment "# Close the
document" and the similar try/except in reload_document) which hides close
failures; update both places (the cached.document.close() call and the close in
reload_document) to catch Exception as e and log the failure at debug (or
warning) level via the module logger before continuing, e.g., include the
exception message and context (path) in the log, then proceed to
server.cache.remove(path) and return result as before.
In `@skills/docx/SKILL.md`:
- Around line 41-46: The optional-parameter `?` suffix in the function
signatures (open_document, close_document) is unclear for Python users; update
the SKILL.md signatures to use Python conventions—either square-bracket notation
(e.g., open_document(path[, author])) or explicit defaults (e.g.,
open_document(path, author=None), close_document(path, force=False))—and apply
the chosen convention consistently to open_document, close_document
(author?/force?) and any other methods with `?` in the shown list
(save_document, reload_document, force_save remain unchanged except for
consistency if needed).
- Line 229: Update the "MCP server" bullet that currently reads "Installed with
plugin (auto-configured)" to explicitly name the plugin, provide install
instructions or a link to the plugin's install docs, and define what
"auto-configured" means (e.g., which config files, default settings applied, and
any post-install steps). Specifically, replace or expand the line referencing
"MCP server" so it mentions the plugin name (e.g., "<plugin-name>"), a one-line
install command or link to installation documentation, and a short sentence
explaining what auto-configuration does and where users can find or override the
generated config (so readers editing SKILL.md can directly follow setup steps).
In `@tests/mcp/test_server.py`:
- Around line 3-4: The test file imports unused symbols: remove the unused
imports `asyncio` and `AsyncMock` from the top import statement and only import
the mocks actually used (e.g., keep `MagicMock` and `patch`) so the import line
matches the symbols referenced in the file (look for usages of `MagicMock` and
`patch` in this test module and remove `asyncio` and `AsyncMock`).
In `@tests/mcp/test_tools.py`:
- Around line 432-466: Update test_accept_revision and test_reject_revision to
fail loudly when no revisions were produced: after calling replace_text and
retrieving revisions via list_revisions, assert that revisions["revisions"] is
not empty (e.g. assert revisions["revisions"], "replace_text did not produce
revisions") before extracting rev_id and calling
accept_revision/reject_revision; this ensures failures in replace_text or
revision generation surface rather than silently skipping assertions.
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (85.3%) is below the target coverage (90.0%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
=======================================
- Coverage 93.6% 85.3% -8.3%
=======================================
Files 10 14 +4
Lines 2133 2849 +716
Branches 485 571 +86
=======================================
+ Hits 1998 2433 +435
- Misses 35 267 +232
- Partials 100 149 +49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 tasks
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docx_editor_mcp/tools.py`:
- Around line 3-13: The logger initialization (logger =
logging.getLogger(__name__)) is placed between imports and breaks the import
block; move that line below all import statements and ensure imports are grouped
and ordered (standard library: logging, os, typing; third-party: docx_editor and
docx_editor.exceptions; local: .cache and .server) so symbols Document,
TextNotFoundError, CachedDocument, normalize_path, and DocxMCPServer are
imported before initializing logger.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docx_editor_mcp/tools.py`:
- Around line 151-157: The cache removal uses the raw input path causing
mismatches with the normalized key used by _get_cached_or_error; change the
removal to use the normalized cache key (e.g., use cached.path or call
normalize_path(path)) instead of the raw path so server.cache.remove(...)
removes the correct entry after closing the document (update both occurrences
around cached.document.close / server.cache.remove and the similar block at the
other location).
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
tests/mcp/test_tools.py (1)
442-446:⚠️ Potential issue | 🟡 MinorRevision tests still skip assertions when no revisions are produced.
Both tests can pass without validating behavior if
revisions["revisions"]is empty, which hides failures in revision generation.Also applies to: 460-463
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_tools.py` around lines 442 - 446, The tests in tests/mcp/test_tools.py currently skip assertions when revisions["revisions"] is empty, hiding failures; update the test to assert that revisions["revisions"] is non-empty before extracting rev_id and calling accept_revision, e.g., add an assertion like assert revisions["revisions"], and do the same fix for the second occurrence (the block around lines 460-463) so the test fails fast when no revisions are produced rather than silently passing.docx_editor_mcp/tools.py (1)
157-157:⚠️ Potential issue | 🟠 MajorRemove cache entries with the normalized key.
Lines 157 and 187 remove by raw
path, but this module consistently uses normalized paths forserver.cache.get(...)andserver.cache.put(...). If a caller passes a relative or differently-cased path, close/reload can leave a closed document behind in the cache, and the nextopen_document()will hand back a stale handle instead of reopening cleanly.♻️ Proposed fix
- server.cache.remove(path) + server.cache.remove(cached.path) @@ - server.cache.remove(path) + server.cache.remove(cached.path)Also applies to: 187-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docx_editor_mcp/tools.py` at line 157, The cache removal calls use the raw path (server.cache.remove(path)) but the module consistently keys cache.get/put with a normalized path; change both removal sites to compute and use the same normalized key before removing (e.g., normalized_path = <the existing normalization helper>(path) and then call server.cache.remove(normalized_path)) so that server.cache.remove uses the identical normalized key used by server.cache.get and server.cache.put.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docx_editor_mcp/cache.py`:
- Around line 109-113: The current insertion path inserts
self._cache[normalized] even if _evict_lru() failed, which can let size exceed
max_documents; update the logic in the methods that perform the capacity check
(the block using normalized, self._cache, self.size, self.max_documents) to only
insert the new document if eviction succeeded or capacity is already available —
e.g., have _evict_lru() return a boolean (or raise) and check its result before
assigning to self._cache[normalized], or loop-calling _evict_lru() until
self.size < self.max_documents; apply the same guard to the other similar block
(lines referenced around 145-153) so insertion is conditional on successful
eviction.
- Around line 179-185: getpass.getuser() may return an empty string; update the
try/except block around getpass.getuser() so after calling getpass.getuser() you
treat empty-string results as missing and fall back to "Reviewer" before
assigning self._session_author and returning, i.e., change the logic in the
block that calls getpass.getuser() to check if default is truthy (non-empty) and
if not set default = "Reviewer" and then assign self._session_author = default
and return default, True.
In `@docx_editor_mcp/server.py`:
- Around line 50-59: The main() function returns immediately after creating the
server, so start its run loop to keep the MCP server alive: after creating the
server with create_server() call its long-running serve method (e.g.,
server.serve_forever() or server.run() / server.start() depending on the server
object's API), wrap that call in a try/except KeyboardInterrupt to log shutdown
and call any server.close()/shutdown() cleanup method, and remove the TODO
placeholder; reference the main function and the create_server()/server object
(and server.name) to locate where to add the blocking serve loop and graceful
shutdown handling.
In `@docx_editor_mcp/tools.py`:
- Around line 308-343: The insert tools currently call
Document.insert_after()/insert_before() without the required keyword-only
paragraph argument, causing a TypeError; update the tool function signatures
(e.g., insert_after and insert_before) to accept a new paragraph: int | None (or
appropriate type), add it to the docstring/Args list, and forward it to the
underlying API call by passing paragraph=paragraph in the call to
cached.document.insert_after(...) and cached.document.insert_before(...); ensure
you preserve existing defaults and mark_dirty()/error handling remains
unchanged.
---
Duplicate comments:
In `@docx_editor_mcp/tools.py`:
- Line 157: The cache removal calls use the raw path (server.cache.remove(path))
but the module consistently keys cache.get/put with a normalized path; change
both removal sites to compute and use the same normalized key before removing
(e.g., normalized_path = <the existing normalization helper>(path) and then call
server.cache.remove(normalized_path)) so that server.cache.remove uses the
identical normalized key used by server.cache.get and server.cache.put.
In `@tests/mcp/test_tools.py`:
- Around line 442-446: The tests in tests/mcp/test_tools.py currently skip
assertions when revisions["revisions"] is empty, hiding failures; update the
test to assert that revisions["revisions"] is non-empty before extracting rev_id
and calling accept_revision, e.g., add an assertion like assert
revisions["revisions"], and do the same fix for the second occurrence (the block
around lines 460-463) so the test fails fast when no revisions are produced
rather than silently passing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 755d5d79-024c-440e-b970-6dc9366b410f
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.claude-plugin/.mcp.jsonREADME.mddocx_editor_mcp/__init__.pydocx_editor_mcp/__main__.pydocx_editor_mcp/cache.pydocx_editor_mcp/server.pydocx_editor_mcp/tools.pyopenspec/changes/add-mcp-server/design.mdopenspec/changes/add-mcp-server/proposal.mdopenspec/changes/add-mcp-server/specs/mcp-server/spec.mdopenspec/changes/add-mcp-server/tasks.mdpyproject.tomlskills/docx/SKILL.mdtests/mcp/__init__.pytests/mcp/test_cache.pytests/mcp/test_server.pytests/mcp/test_tools.py
Add proposal for persistent DOM caching via MCP server to improve performance for interactive document editing sessions. Key decisions: - Separate docx_editor_mcp/ package (optional dependency) - LRU cache with 10 document limit - mtime-based external change detection - Session author memory with cross-platform default - Path normalization for consistent cache keys - Plugin auto-configuration via .mcp.json - MCP-first skill approach
Implements OpenSpec tasks 1.1-1.6: - Create docx_editor_mcp/ package structure - DocumentCache with configurable max documents - CachedDocument with mtime tracking for external change detection - Path normalization (resolves ~, relative paths, symlinks) - LRU eviction with save-on-evict for dirty documents - Session author memory with cross-platform default (getpass.getuser) All 24 cache tests pass, no regressions (513 total tests pass).
Implements OpenSpec tasks 2.1-2.2: - DocxMCPServer wrapper with DocumentCache - create_server() factory function - Graceful shutdown saves all dirty documents (best-effort) - main() entry point for CLI 8 new tests, 32 total MCP tests pass.
Implements OpenSpec task 3.1-3.5: - Document lifecycle: open, save, close, reload, force_save - Track changes: replace_text, delete_text, insert_after, insert_before - Comments: add, list, reply, resolve, delete - Revisions: list, accept, reject, accept_all, reject_all - Read tools: find_text, count_matches, get_visible_text - External change detection blocks edits when file modified 40 new tests, 72 total MCP tests pass.
- Add [project.optional-dependencies] with mcp>=1.0.0 - Add [project.scripts] entry point: mcp-server-docx - Add __main__.py for python -m docx_editor_mcp - Update wheel packages to include docx_editor_mcp - Update coverage and isort settings for new package
- Document all MCP tools: lifecycle, track changes, comments, revisions, read - Add workflow examples for document review - Document external change detection and best practices - Remove Python script examples for editing (now uses MCP tools) - Keep pandoc and python-docx for reading/navigation
- Document installation with [mcp] extra - Document running the server - Document Claude Code configuration (manual and auto) - List all available MCP tools - Update tasks.md marking all sections complete
- Handle save failure during LRU eviction (don't evict if save fails) - Normalize path in CachedDocument.__post_init__ for consistency - Add logging for eviction errors - Fix type annotations for proper type narrowing
- Add debug logging when document.close() fails in close_document/reload_document - Use logger.exception() instead of logger.error() for better tracebacks in shutdown
Add openspec proposal for search_text, get_paragraph_text, get_document_info, paginated list_paragraphs, and smart get_visible_text truncation to support large document workflows.
…raphs - Complete FastMCP server wiring with lifespan, tool registration, and stdio transport - Add list_paragraphs and batch_edit MCP tools - Update all track changes tools to require paragraph hash-anchored refs - Rename tests/mcp/ to tests/test_mcp/ to fix module shadowing with mcp package - Add tests for FastMCP app creation and async lifespan
- Edit tools now return new_ref (paragraph ref) instead of change_id - Add rewrite_paragraph and batch_rewrite MCP tools - Update tests for new return key names
Core library: - search_text(query, context_chars) with paragraph refs and context - get_paragraph_text(paragraphs) to read specific paragraphs by ref - get_document_info() for paragraph count, word count, heading outline - list_paragraphs now supports start/limit pagination MCP layer: - Register search_text, get_paragraph_text, get_document_info tools - get_visible_text auto-truncates at 10K chars with exploration hint - list_paragraphs returns total count for pagination - Updated SERVER_INSTRUCTIONS to guide LLMs toward exploration workflow
…tools These tool functions were added in this PR but had zero test coverage. Skipped: AssertionError "typo" (it's the correct Python builtin name), result variable reuse pattern (pre-existing, works at runtime).
- Raise RuntimeError when cache eviction fails (prevents capacity violation) - Handle empty getpass.getuser() with fallback to "Reviewer" - Use cached.path (normalized) in remove() calls for consistency
CI doesn't install the [mcp] extra, so tests importing from mcp.server.fastmcp fail with ModuleNotFoundError. Add skipif markers to TestFastMCPApp and TestLifespan classes. Also includes ruff format fix for document.py line length.
- Remove unused list_paragraphs import in test_tools.py - Add ty: ignore[unresolved-import] for mcp imports (optional dep)
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
1 participant
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.
Summary
docx_editor_mcp) for persistent DOM caching between tool calls.mcp.jsonChanges
docx_editor_mcp/package with cache, server, and tools[mcp]optional dependency andmcp-server-docxentry pointTest plan
pip install .[mcp]mcp-server-docxandpython -m docx_editor_mcpuvx mcp-server-docxafter PyPI publish🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores