Complete issue #99 Phase 3: rewire MCP to the public edit core#105
Open
Complete issue #99 Phase 3: rewire MCP to the public edit core#105
Conversation
Contributor
📝 WalkthroughWalkthroughRestructures the codebase to make Changes
Sequence Diagram(s)sequenceDiagram
actor Host
participant MCP_Service as MCP Service<br/>(mcp.patch.service)
participant Edit_Service as Edit Service<br/>(edit.service)
participant Edit_Runtime as Edit Runtime<br/>(edit.runtime)
participant Engine_COM as COM Engine<br/>(xlwings)
participant Engine_Open as OpenPyXL Engine<br/>(openpyxl)
Host->>MCP_Service: run_patch(request, policy)
activate MCP_Service
MCP_Service->>MCP_Service: _resolve_patch_request_paths(policy)
MCP_Service->>Edit_Service: patch_workbook(request)
deactivate MCP_Service
activate Edit_Service
Edit_Service->>Edit_Runtime: resolve_input_path(), resolve_output_path()
Edit_Service->>Edit_Runtime: select_patch_engine(request, input_path, com_available)
Edit_Runtime-->>Edit_Service: PatchEngine
alt COM available
Edit_Service->>Engine_COM: apply_xlwings_engine(...)
Engine_COM-->>Edit_Service: result
else fallback/COM error
Edit_Service->>Engine_Open: apply_openpyxl_engine(...)
Engine_Open-->>Edit_Service: result
end
Edit_Service->>Edit_Service: _coerce_model_list(result) → PatchResult
Edit_Service-->>MCP_Service: PatchResult
deactivate Edit_Service
MCP_Service-->>Host: PatchResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+230
to
+235
| except ValueError: | ||
| raise | ||
| except FileNotFoundError: | ||
| raise | ||
| except OSError: | ||
| raise |
There was a problem hiding this comment.
🟡 Zero-byte placeholder file not cleaned up on ValueError/FileNotFoundError/OSError in openpyxl path
The new next_available_path in edit/output_path.py:94-106 atomically creates zero-byte placeholder files when on_conflict="rename". The _apply_with_openpyxl function in edit/service.py properly cleans up these placeholders in PatchOpError (line 220), general Exception (line 237), dry_run (line 281), and preflight error (line 270) paths. However, the ValueError, FileNotFoundError, and OSError re-raise paths at lines 230-235 skip cleanup, leaving the zero-byte reservation file on disk. The old code in mcp/patch/service.py had the same re-raise pattern but wasn't affected because the old next_available_path in mcp/shared/output_path.py:82-92 used an exists() check without creating files.
Suggested change
| except ValueError: | |
| raise | |
| except FileNotFoundError: | |
| raise | |
| except OSError: | |
| raise | |
| except ValueError: | |
| _cleanup_empty_reserved_output(reserved_output_path) | |
| raise | |
| except FileNotFoundError: | |
| _cleanup_empty_reserved_output(reserved_output_path) | |
| raise | |
| except OSError: | |
| _cleanup_empty_reserved_output(reserved_output_path) | |
| raise |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
This PR completes Issue #99 Phase 3 by making exstruct.edit the canonical workbook editing core and rewiring MCP patch/make execution to call into that public core while preserving legacy MCP compatibility (including monkeypatch-based overrides).
Changes:
- Move canonical editing orchestration/models/runtime/engine boundaries into
src/exstruct/edit/**and removeexstruct.mcp.*imports from the edit package. - Rework MCP patch/make shims (
mcp.patch_runner,mcp.patch.service,mcp.patch.runtime,mcp.patch.models,mcp.patch.engine.*) to delegate to the edit core while preserving legacy monkeypatch surfaces/override precedence. - Add/adjust tests and documentation to lock in the new layering boundary and compatibility behavior.
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/exstruct/edit/service.py | Canonical patch/make orchestration (engine selection, fallback, conflict handling, preflight attribution). |
| src/exstruct/edit/runtime.py | Policy-free runtime helpers backing the edit service. |
| src/exstruct/edit/models.py | Canonical Pydantic editing models moved into exstruct.edit. |
| src/exstruct/edit/output_path.py | Edit-owned output/conflict helpers with atomic rename reservation. |
| src/exstruct/edit/a1.py | Edit-owned A1 parsing/geometry helpers (decoupled from MCP shared utils). |
| src/exstruct/edit/engine/__init__.py | Public engine boundary exports for the edit core. |
| src/exstruct/edit/engine/openpyxl_engine.py | Canonical openpyxl engine boundary calling edit-owned internal implementation. |
| src/exstruct/edit/engine/xlwings_engine.py | Canonical xlwings engine boundary calling edit-owned internal implementation. |
| src/exstruct/edit/api.py | Public API wrappers for patch/make entry points. |
| src/exstruct/edit/errors.py | Edit-owned PatchOpError re-export (now from edit internal). |
| src/exstruct/edit/normalize.py | Switch to edit-owned A1 helpers (parse_range_geometry). |
| src/exstruct/mcp/patch_runner.py | Sync legacy overrides (incl. COM availability) into edit runtime/internal and MCP shims. |
| src/exstruct/mcp/patch/service.py | Compatibility wrapper that injects legacy monkeypatch targets via live module lookup and delegates to edit service. |
| src/exstruct/mcp/patch/runtime.py | Legacy runtime shim re-exporting edit runtime helpers + policy-aware path wrappers. |
| src/exstruct/mcp/patch/models.py | Convert MCP patch models module into a thin compatibility re-export of edit models. |
| src/exstruct/mcp/patch/ops/common.py | PatchOpError import now points to edit-owned error type. |
| src/exstruct/mcp/patch/engine/openpyxl_engine.py | Legacy engine import path wrapper maintained. |
| src/exstruct/mcp/patch/engine/xlwings_engine.py | Legacy engine import path wrapper maintained. |
| tests/edit/test_edit_service.py | New edit-core service tests (COM preference, fallback, preflight attribution, rename reservation cleanup). |
| tests/edit/test_edit_output_path.py | Tests for atomic rename reservation and directory policy ordering. |
| tests/edit/test_architecture.py | Enforces “edit has no mcp imports” and “import exstruct.edit doesn’t load exstruct.mcp”. |
| tests/edit/test_api.py | Verifies model import identity across edit and MCP compatibility paths. |
| tests/mcp/test_patch_runner.py | Regression test for patch_runner COM availability override propagation into edit core. |
| tests/mcp/test_make_runner.py | Regression tests for override propagation reaching .xls make validation path. |
| tests/mcp/patch/test_service.py | Regression tests ensuring legacy engine monkeypatch points still work through shims. |
| tests/mcp/patch/test_runtime_shim.py | Tests for legacy runtime shim policy= kwarg surface. |
| tests/mcp/patch/test_legacy_runner_ops.py | Legacy runner tests updated to target edit-owned internals + override sync regression. |
| docs/mcp.md | Clarifies MCP as host/integration layer; points internal layering to dev docs. |
| dev-docs/specs/editing-api.md | Updates spec to reflect edit core ownership and MCP compatibility boundaries. |
| dev-docs/specs/data-model.md | Updates model ownership/module locations to exstruct.edit.models. |
| dev-docs/architecture/overview.md | Updates architecture overview to reflect new edit core modules and MCP shim roles. |
| tasks/todo.md | Tracks review follow-up actions and verification commands for this PR. |
| tasks/lessons.md | Adds lessons on pytest basename collisions and compat shim monkeypatch strategy. |
| tasks/feature_spec.md | Records accepted findings/constraints/test plan for the Phase 3 follow-up work. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/exstruct/edit/output_path.py (1)
76-91:⚠️ Potential issue | 🟠 Major
renamemode still has a race window when the target file does not yet exist.Line 76 returns early on non-existence, so
on_conflict="rename"skips atomic reservation in that path. Concurrent callers can still choose the same output and collide.🔧 Suggested direction
def apply_conflict_policy( output_path: Path, on_conflict: OnConflictPolicy ) -> tuple[Path, str | None, bool]: """Apply output conflict policy to a resolved output path.""" - if not output_path.exists(): - return output_path, None, False + if on_conflict == "rename": + reserved = next_available_path(output_path) + if reserved == output_path.resolve(): + return reserved, None, False + return ( + reserved, + f"Output exists; renamed to: {reserved.name}", + False, + ) + if not output_path.exists(): + return output_path, None, False if on_conflict == "skip": return ( output_path, f"Output exists; skipping write: {output_path.name}", True, ) - if on_conflict == "rename": - renamed = next_available_path(output_path) - return ( - renamed, - f"Output exists; renamed to: {renamed.name}", - False, - ) return output_path, None, False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/edit/output_path.py` around lines 76 - 91, The race happens because when on_conflict == "rename" the code returns early if output_path doesn't exist and skips an atomic reservation; change the logic so rename always attempts an atomic reservation of the chosen target (whether it already exists or not) before returning. Specifically, after selecting the candidate path (use next_available_path(output_path) when needed), attempt to atomically reserve/create it (e.g., using exclusive creation semantics, os.open with O_CREAT|O_EXCL or open(..., "x") semantics) and only return that reserved path; if reservation fails because the name was taken, loop to the next_available_path and retry. Keep references to output_path, on_conflict and next_available_path in the fix.
🧹 Nitpick comments (1)
tests/edit/test_architecture.py (1)
26-38: Good subprocess-based runtime check.The isolation approach correctly verifies that
exstruct.mcpis not transitively loaded at runtime. Usingsys.executableensures the same interpreter is used.Consider adding a
timeoutparameter to prevent indefinite hangs in edge cases (e.g., import-time deadlocks):🔧 Optional: Add timeout for robustness
result = subprocess.run( [ sys.executable, "-c", "import exstruct.edit, sys; print('exstruct.mcp' in sys.modules)", ], check=False, capture_output=True, text=True, + timeout=30, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/edit/test_architecture.py` around lines 26 - 38, The test function test_import_exstruct_edit_does_not_load_mcp_package should add a timeout to the subprocess.run invocation to avoid potential indefinite hangs; update the call in that test (the subprocess.run inside test_import_exstruct_edit_does_not_load_mcp_package) to include a reasonable timeout parameter (e.g., timeout=10) so the subprocess fails fast on deadlocks while preserving the existing check/capture_output/text behavior and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/exstruct/mcp/patch/service.py`:
- Around line 53-60: The monkeypatch in _sync_compat_overrides doesn't affect
calls because edit_service.patch_xlwings and patch_workbook use locally imported
references apply_openpyxl_engine/apply_xlwings_engine captured at module import;
update edit_service so those functions perform attribute lookup on the module
(e.g., call edit_service.apply_xlwings_engine /
edit_service.apply_openpyxl_engine or getattr(edit_service, ...) at call time)
instead of using the direct imports, or change the import style in edit_service
to import the engine module and call
module.apply_xlwings_engine/module.apply_openpyxl_engine; ensure patch_xlwings
and patch_workbook reference the module attribute names so the assignments in
_sync_compat_overrides take effect.
---
Duplicate comments:
In `@src/exstruct/edit/output_path.py`:
- Around line 76-91: The race happens because when on_conflict == "rename" the
code returns early if output_path doesn't exist and skips an atomic reservation;
change the logic so rename always attempts an atomic reservation of the chosen
target (whether it already exists or not) before returning. Specifically, after
selecting the candidate path (use next_available_path(output_path) when needed),
attempt to atomically reserve/create it (e.g., using exclusive creation
semantics, os.open with O_CREAT|O_EXCL or open(..., "x") semantics) and only
return that reserved path; if reservation fails because the name was taken, loop
to the next_available_path and retry. Keep references to output_path,
on_conflict and next_available_path in the fix.
---
Nitpick comments:
In `@tests/edit/test_architecture.py`:
- Around line 26-38: The test function
test_import_exstruct_edit_does_not_load_mcp_package should add a timeout to the
subprocess.run invocation to avoid potential indefinite hangs; update the call
in that test (the subprocess.run inside
test_import_exstruct_edit_does_not_load_mcp_package) to include a reasonable
timeout parameter (e.g., timeout=10) so the subprocess fails fast on deadlocks
while preserving the existing check/capture_output/text behavior and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50ee031e-f933-45d0-be67-3e05f1417881
📒 Files selected for processing (17)
docs/mcp.mdsrc/exstruct/edit/models.pysrc/exstruct/edit/output_path.pysrc/exstruct/edit/runtime.pysrc/exstruct/edit/service.pysrc/exstruct/mcp/patch/internal.pysrc/exstruct/mcp/patch/runtime.pysrc/exstruct/mcp/patch/service.pysrc/exstruct/mcp/patch_runner.pytasks/feature_spec.mdtasks/todo.mdtests/edit/test_architecture.pytests/edit/test_edit_output_path.pytests/edit/test_edit_service.pytests/mcp/patch/test_legacy_runner_ops.pytests/mcp/patch/test_runtime_shim.pytests/mcp/test_make_runner.py
✅ Files skipped from review due to trivial changes (1)
- tasks/feature_spec.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/mcp.md
- tests/edit/test_edit_service.py
- tests/mcp/test_make_runner.py
Comment on lines
+53
to
+60
| def _sync_compat_overrides() -> None: | ||
| """Propagate legacy monkeypatch targets into the canonical edit service.""" | ||
| service_module = cast(Any, edit_service) | ||
| runtime_module = cast(Any, edit_runtime) | ||
| service_module.apply_openpyxl_engine = apply_openpyxl_engine | ||
| service_module.apply_xlwings_engine = apply_xlwings_engine | ||
| runtime_module.get_com_availability = runtime.get_com_availability | ||
| runtime_module.PatchOpError = runtime.PatchOpError |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how apply_openpyxl_engine and apply_xlwings_engine are called within edit_service.py
rg -n "apply_openpyxl_engine|apply_xlwings_engine" src/exstruct/edit/service.py -A2 -B2Repository: harumiWeb/exstruct
Length of output: 600
Monkeypatch won't affect captured references: apply_openpyxl_engine and apply_xlwings_engine are called directly in edit_service.py.
Lines 57–58 assign these functions to service_module attributes, but they are imported via from .engine... statements at module load time (lines 12–13). The actual calls inside patch_xlwings (line 213) and patch_workbook (line 121) use the captured local references, not module attribute lookup, so the monkeypatch won't propagate to those internal calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/exstruct/mcp/patch/service.py` around lines 53 - 60, The monkeypatch in
_sync_compat_overrides doesn't affect calls because edit_service.patch_xlwings
and patch_workbook use locally imported references
apply_openpyxl_engine/apply_xlwings_engine captured at module import; update
edit_service so those functions perform attribute lookup on the module (e.g.,
call edit_service.apply_xlwings_engine / edit_service.apply_openpyxl_engine or
getattr(edit_service, ...) at call time) instead of using the direct imports, or
change the import style in edit_service to import the engine module and call
module.apply_xlwings_engine/module.apply_openpyxl_engine; ensure patch_xlwings
and patch_workbook reference the module attribute names so the assignments in
_sync_compat_overrides take effect.
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.
Summary
exstruct.editcore.exstruct.editphysically independent fromexstruct.mcp.*while preserving legacy MCP compatibility paths.patch_runner.get_com_availabilityand legacymcp.patch.engine.*monkeypatches still work through the compatibility layer.Acceptance Criteria (Issue #99 Phase 3)
exstruct.editis the canonical editing core for models, runtime, engines, and service orchestration.src/exstruct/edit/**does not importexstruct.mcp.*.mcp.patch_runner,mcp.patch.service,mcp.patch.runtime,mcp.patch.internal, and monkeypatch-based tests.Validation
uv run pytest tests/edit -quv run pytest tests/mcp/test_patch_runner.py tests/mcp/test_make_runner.py tests/mcp/test_tools_handlers.py tests/mcp/test_server.py tests/mcp/patch -quv run task precommit-rundocs/mcp.md, internal specs/architecture docs).Notes
3d8eacbdoc/仕様とタスク定義8924ca9Implement edit core phase 3 rewiring7467ac7Refine edit core MCP decoupling and compat shimsSummary by CodeRabbit
New Features
Enhancements
Documentation
Tests